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

[Michael Tokarev <mjt@tls.msk.ru>]

04.11.2014 16:39, Alex Bennée wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
> 
>> Convert several Character backend open functions to use the Error API.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>  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.

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

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.

P.S.  Alex, please trim unrelated original text in your replies
a bit, -- it is kinda easy to miss your small comments scattered
in a huge original patch.  (Hopefully I didn't miss any)

Thanks,

/mjt