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

[zhanghailiang]

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