Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 4/5] qemu-char: convert some open functions to use Error API

[Markus Armbruster]

Michael Tokarev <address@hidden> writes:

> 04.11.2014 16:39, Alex Bennée wrote:
>> zhanghailiang <address@hidden> writes:
>> 
>>> Convert several Character backend open functions to use the Error API.
>>>
>>> Signed-off-by: zhanghailiang <address@hidden>
>>> ---
>>>  qemu-char.c | 76 
>>> +++++++++++++++++++++++++++++++++----------------------------
>>>  1 file changed, 41 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/qemu-char.c b/qemu-char.c
>>> index 0f38cdd..a1d25c7 100644
>>> --- a/qemu-char.c
>>> +++ b/qemu-char.c
>>> @@ -1077,7 +1077,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, 
>>> int fd_out)
>>>      return chr;
>>>  }
>>>  
>>> -static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
>>> +static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts, Error 
>>> **errp)
>>>  {
>>>      int fd_in, fd_out;
>>>      char filename_in[CHR_MAX_FILENAME_SIZE];
>> <snip>
>> Why convert the call if we are not using the passed parameter?
>
> This is actually a good question, -- one way or another.  On one hand,
> this way we're making it all consistent for the caller at least.  On
> another, this, at least, introduces a warning about unused parameter,
> so an 'unused' attribute might be a good idea.

Matter of taste.

As long as the open functions remain different, I don't see the value of
adding unused Error ** parameters.

The churn would save a purpose if it actually unified the function types
and simplified the switch to an indirect call.

Marking unused parameters with __attribute__((unused)) is a good idea
indeed.

> []
>>> -static CharDriverState *qemu_chr_open_win_path(const char *filename)
>>> +static CharDriverState *qemu_chr_open_win_path(const char *filename,
>>> +                                               Error **errp)
>>>  {
>>>      CharDriverState *chr;
>>>      WinCharState *s;
>>> +    Error *local_err = NULL;
>>>  
>>>      chr = qemu_chr_alloc();
>>>      s = g_malloc0(sizeof(WinCharState));
>>> @@ -2033,9 +2035,11 @@ static CharDriverState *qemu_chr_open_win_path(const 
>>> char *filename)
>>>      chr->chr_write = win_chr_write;
>>>      chr->chr_close = win_chr_close;
>>>  
>>> -    if (win_chr_init(chr, filename) < 0) {
>>> +    win_chr_init(chr, filename, &local_err);
>>> +    if (local_err) {
>>>          g_free(s);
>>>          g_free(chr);
>>> +        error_propagate(errp, local_err);
>>>          return NULL;
>> 
>> Hmm I'm not sure I find the change from a return value to
>> pass-by-reference return value intuitive. What does this gain us?
>> 
>> Are the messages now being reported actually more suitable for user
>> consumption? For example "ClearCommError failed" doesn't actually tell
>> the user much apart from something went wrong.
>
> Alex, I think you're way too late into the game already.  This
> error api has been designed this way quite some time ago, and
> many places uses it this way.  I don't really like it too, but
> heck, what can I do?

I'm not really happy with it either, but it addresses real problems.

Down where the error happens, you know what went wrong, but have no idea
how to handle it.

Further up where you know how to handle errors, you have no idea what
went wrong.

Passing up success/failure doesn't help with that.

Passing up -errno helps in simple cases.

Inventing other error enumerations tends to produce a mess and/or end in
confusion.

Passing up an error *object* can provide information on what went wrong.
It's admittedly cumbersome, and prone to leak memory.  I prefer to stick
to -errno in simple cases.

Our current object is basically a container for a human-readable error
message for error handling to use.

Example: when win_chr_init() fails, it prints to stderr.  This is
actually wrong when it runs on behalf of monitor command chardev-add.
It should print to the monitor when it's HMP, and should send an error
reply when it's QMP.  win_chr_init() doesn't know.  The monitor calling
qmp_chardev_add() does.

> I don't actually get it why, when converting some function which
> returned success/failure before, to this error API, the return
> value is always discarded and the function becomes void?  I'd
> keep the return value (success/failure) _and_ the error, to
> have better shugar in places like this one.  But I guess it'll
> be a bit more confusing, which condition should be treated as
> error - failure function return or non-null error argument.

Yes, this annoys me, too.  It can turn a simple

    if (frobnicate(arg) < 0) {
        goto fail;
    }

into

    Error *local_err = NULL;

    frobnicate(arg, &local_err);
    if (local_err) {
        error_free(local_err);
        goto fail;
    }

when it could be simply

    if (frobnicate(arg, NULL) < 0) {
        goto fail;
    }

> But this is. again, not about this patch/change -- this is how
> qemu is doing for quite some time, discusing/changing this
> should be elsewhere.

Moaning about it in review is fine (I indulge in such things myself from
time to time), but if you want infrastructure changed, you probably have
to start with a patch changing it.

[...]