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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2014/11/5 15:05, Michael Tokarev wrote:
> 04.11.2014 16:25, Alex Bennée wrote:
>> zhanghailiang <zhang.zhanghailiang@huawei.com> 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 <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>> ---
>>>   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.
process:
   qmp_call_cmd
       --->qmp_marshal_input_chardev_add
           --->qmp_chardev_add
             --->qemu_chr_open_pipe

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

{"id":"libvirt-12","error":{"class":"GenericError",\
"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.
>
> Thanks,
>
> /mjt
>
> .
>