Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions

[Michael Tokarev]

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?

Yes, the code first calls parse_pipe() and only after it is
successfully completed, it calls open_pipe().  I don't see
a good reason for having assert here.

> 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.

Thanks,

/mjt