04.11.2014 16:25, Alex Bennée wrote:
> zhanghailiang <zhang.zhanghaili...@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.zhanghaili...@huawei.com>
>> Signed-off-by: Michael Tokarev <m...@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?
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
