[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 4/5] qemu-char: convert some o
From: |
Michael Tokarev |
Subject: |
Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 4/5] qemu-char: convert some open functions to use Error API |
Date: |
Wed, 05 Nov 2014 10:15:57 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.2.0 |
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.
[]
>> -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
- Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 3/5] qemu-char: fix incorrect state in error message, (continued)
[Qemu-devel] [PATCH v3 4/5] qemu-char: convert some open functions to use Error API, zhanghailiang, 2014/11/04
[Qemu-devel] [PATCH v3 2/5] spice-qemu-char: fix parameter checks in qemu_chr_parse_* functions, zhanghailiang, 2014/11/04