Re: [Qemu-devel] [PATCH v2 0/5] Trivial patch about qemu-char

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

On 2014/11/3 22:10, Michael Tokarev wrote:
> 03.11.2014 14:39, zhanghailiang wrote:
>> On 2014/11/3 18:03, Michael Tokarev wrote:
>>> 03.11.2014 12:44, zhanghailiang wrote:
>>>> Patch 1~3 fix wrong check about in-parameter.
>>>> The last two patches convert some open functions to use Error API.
>>>>
>>>> v2:
>>>> - don't use error_setg when followed by exit(), it does not report an error (Eric Blake)
>>>> - check the parameter in qemu_chr_parse_* functions and remove the check in qemu_chr_open_* functions. (Michael Tokarev)
>>>>
>>>> Thanks very much for their reviews and suggestions;)
>>>
>>> Thank you for doing all this.  I think I can apply this but with folding
>>> patches 1 and 2 into one, -- it is better to see them both in the same
>>> context, just like you did in patch 3.  If that's okay with you, I'll
>>> just apply it like this.
>>
>> Sure, I'm Ok with it;)
>
> I myself also prefer to use construct like !str[0] in place of strlen(str) != 0,
> to mean we check for its emptiness, we don't really need its lenght.  But it is

Hmm, Sounds good, will fix like that in V3.

> not a probem at all, just a matter of persona preference.
>
> I suggest mergeing the two commits into one, and also fix grammar erros
> in the commit message, something like this (fell free to use this
> commit message to the combined commit):
>

OK;)

>      qemu-char: fix parameter check in some qemu_chr_parse_* functions
>
>      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 we should find the error by checking parameter length, just like
>      what qemu_chr_parse_udp does, it will help to find what exactly is
>      wrong.
>
>      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>
>
>
> [error API]
>>
>> Yes, this confused me, besides, error_setg will add a '\n' in the end, but
>> for example, print_allowed_subtypes where use fprintf(stderr,..) without '\n' in the middle places,
>> IMHO, maybe it makes sense to return this to the higher caller.
>
> Oh.  I overlooked this one.  Indeed, this should not be done this way,
> you can't print_allowed_subtypes() in there _before_ reporting actual
> error, that'd be wrong.  Oh well :)
>
> In this context, print_allowed_subtypes() should return a single string
> with all subtypes in it.  I don't think it is interesting to print this
> allowed_subtypes list in case no subtype is specified to start with,
> but for invalid subtype it can be handled by adding the list to the
> original error message (note we already have the list handy in this
> function).  Something like this (on top of the original code, untested):
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 8106e06..0acc9e5 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -244,23 +244,6 @@ static void spice_chr_fe_event(struct CharDriverState *chr, int event)
>   #endif
>   }
>
> -static void print_allowed_subtypes(void)
> -{
> -    const char** psubtype;
> -    int i;
> -
> -    fprintf(stderr, "allowed names: ");
> -    for(i=0, psubtype = spice_server_char_device_recognized_subtypes();
> -        *psubtype != NULL; ++psubtype, ++i) {
> -        if (i == 0) {
> -            fprintf(stderr, "%s", *psubtype);
> -        } else {
> -            fprintf(stderr, ", %s", *psubtype);
> -        }
> -    }
> -    fprintf(stderr, "\n");
> -}
> -
>   static CharDriverState *chr_open(const char *subtype,
>       void (*set_fe_open)(struct CharDriverState *, int))
>
> @@ -288,21 +271,31 @@ static CharDriverState *chr_open(const char *subtype,
>
>   CharDriverState *qemu_chr_open_spice_vmc(const char *type)
>   {
> -    const char **psubtype = spice_server_char_device_recognized_subtypes();
> +    const char **subtype_list = spice_server_char_device_recognized_subtypes();
> +    const char **psubtype;
>
>       if (type == NULL) {
>           fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> -        print_allowed_subtypes();
>           return NULL;
>       }
> -    for (; *psubtype != NULL; ++psubtype) {
> +    for (psubtype = subtype_list; *psubtype != NULL; ++psubtype) {
>           if (strcmp(type, *psubtype) == 0) {
>               break;
>           }
>       }
>       if (*psubtype == NULL) {
> -        fprintf(stderr, "spice-qemu-char: unsupported type: %s\n", type);
> -        print_allowed_subtypes();
> +        char *subtypes;
> +        int len;
> +        for(len = 1, psubtype = subtype_list; *psubtype; ++psubtype) {
> +            len += strlen(*psubtype) + (psubtypes <> psubtype);
> +        }
> +        subtypes = g_alloc(len);
> +        for(len = 0, psubtype = subtype_list; *psubtype; ++psubtype) {
> +            len += sprintf(subtypes + len, "%s%s",
> +                           psubtypes <> psubtype ? "," : "", *psubtype);
> +        }
> +        fprintf(stderr, "spice-qemu-char: unsupported type: %s, allowed types: %s\n", type, subtypes);
> +        g_free(subtypes);
>           return NULL;
>       }
> []
> In another reply, to patch 5:
>
>>> Now this is funny.  Why we have two functions nearby using different
>>> error reporting APIs?  Maybe qemu_chr_open_spice_port() should be
>>> converted to Error API too, at the same time (maybe in the same
>>> patch or in a subsequent patch in the same series)?
>>
>> Actually, after patch 3, there will be no error case for this function, it can not
>> fail, so i just leave it. What's your opinion? Thanks.
>
> Okay, I haven't realized it.  Yes, that makes sense.
>
> []
>> It's okay, So what's your opinion about this series?
>> Should i merge patch 1 and 2 into one patch in V3?
>> Keep the other modifies except the incorrect modify in qemu_chr_open_pty for patch 4?
>> If yes, i will send V3;)
>
> I was ready to merge these but realized I shouldn't because that'll be
> too much on my part, only adding to your confusion - which part is
> applied and which is not, etc.
>
> So please do a V3, to have common base for all this.
>

OK;)

> I'm not sure if the print_allowed_subtypes() thing is a good idea actually --
> it is a bit too large, but without it we can't just convert things as you
> tried to do, for hopefully obvious reasons.  Maybe asking spice maintainer
> for the opinion is a good idea? (Cc'ing Gerd).  The context is, for example,
> here: http://lists.nongnu.org/archive/html/qemu-devel/2014-11/msg00187.html .
>

print_allowed_subtypes() will be used where there is a check error type given(as you said above),
I'd better keep it, but use error_printf (suggested by Markus Armbruster in another email:))
instead of fprintf. I think in this way it is more clean;)

> This patch also has somewhat large list of conversions of other errors in
> windows part of the code, and these all really look terrible, this is not
> correct english really, as far as I know -- it is either "Failed _to_ do
> something", or "something failed", but not "failed something".. ;)  I'm

Yep, needs some little fix for that. Will do in V3.

> not, again, sure that's a good idea to leave it as it is, maybe at least
> some context in the error message will help... ;)
>

Thanks,
zhanghailiang