[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 1/5] qemu-char: fix parameter

From: zhanghailiang
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions
Date: Wed, 5 Nov 2014 20:19:20 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Thunderbird/31.1.1

On 2014/11/5 15:05, Michael Tokarev wrote:
04.11.2014 16:25, Alex Bennée wrote:
zhanghailiang <address@hidden> writes:

For some qemu_chr_parse_* functions, we just check whether the parameter
is NULL or not, but do not check if it is empty.

For example:
qemu-system-x86_64 -chardev pipe,id=id,path=
It will pass the check of NULL but will not find the error until
trying to open it, while essentially missing and empty parameter
is the same thing.

So check the parameters for emptiness too, and avoid emptiness
check at open time.

Signed-off-by: zhanghailiang <address@hidden>
Signed-off-by: Michael Tokarev <address@hidden>
  qemu-char.c | 15 +++++----------
  1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index bd0709b..a09bbf6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1084,11 +1084,6 @@ static CharDriverState 
*qemu_chr_open_pipe(ChardevHostdev *opts)
      char filename_out[CHR_MAX_FILENAME_SIZE];
      const char *filename = opts->device;

-    if (filename == NULL) {
-        fprintf(stderr, "chardev: pipe: no filename given\n");
-        return NULL;
-    }

You seem to have dropped a check here, are you sure all avenues into
this code have validated filename? What if a new function gets added?

Hi Michael,

Yes, the code first calls parse_pipe() and only after it is
successfully completed, it calls open_pipe().  I don't see

Unfortunately :( , That's right for hmp command 'chardev-add' and
startUp configure, but not true for qmp command 'chardev-add'.
It is my fault, i didn't test qmp command before :(

The call process is different from hmp command,
Its route not include parse_* function.

test & result:
{ "execute" : "chardev-add","arguments" : { "id" : "bar1","backend" : \
{ "type" : "pipe","data" : {"device" :"" } } } }

"desc":"Failed to create chardev"}}

As you see, we still need check if filename is empty or not in open_pipe.
(Actually, filename will still never to be NULL,
it is assured by the 'qmp_marshal ' layer, but better to keep it there)

So what's your suggestion? Keep two checks both in open_* and parse_*?
Or move check into open_*? (It should be OK to g_strdup(NULL)). Thanks.

a good reason for having assert here.

Agreed, assert here is still not unnecessary,
filename will never to be NULL in these two cases.

At a minimum I'd replace it with a g_assert(filename) to make the
calling contract clear.

This is an internal set of APIs for a chr device, each kind is
having a pair of functions which are called in order (first parse,
next open), -- _that_ is the contract.

All this boilerplate checking makes me think that either the qemu_opt
machinery should be ensuring we get a valid option string?

Might be a good idea, yes, but that'd be a huge change, since that
should be done in a lot of places, and in many cases we can't
express our rules easily (eg, only one of two parameters should
be present).  I think at this stage adding simple checks to
_parse functions is the way to go, and it is easy to read too.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]