[Qemu-devel] [PATCH v3 0/5] Trivial patch about qemu-char

[Qemu-devel] [PATCH v3 0/5] Trivial patch about qemu-char

[zhanghailiang]

Patch 1 and 2 fix check about parameter in chr_parse_* functions.
Patch 3 fix wrong english state in error message in windows part of the code.
The last two patches convert some open functions to use Error API.

In patch 4, i have tried to convert some codes which will be used 
in windows hypervisor. I didn't test them. But this is not too complex.

Markus has suggested add a separate patch for print_allowed_subtypes to use
error_prinf, but i think it's also in the context of "use Error API", so 
i fix it togther in patch 5.

v3:
- discard wrong modify about qemu_chr_open_pty (Michael Tokarev)
- use !str[0] in place of strlen(str) == 0 which makes more sense for parameter check (Michael Tokarev)
- Merge v2's patch 1 and 2 into a single patch (Michael Tokarev)
- Add new patch 3 to fix wrong english state in error message
- Use error_prinf instead of fprint for print_allowed_subtypes (Markus Armbruster) 

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;)

zhanghailiang (5):
  qemu-char: fix parameter check in some qemu_chr_parse_* functions
  spice-qemu-char: fix parameter checks in qemu_chr_parse_* functions
  qemu-char: fix incorrect state in error message
  qemu-char: convert some open functions to use Error API
  spice-qemu-char: convert some functions to use Error API

 include/ui/qemu-spice.h     |  2 +-
 qemu-char.c                 | 93 +++++++++++++++++++++++----------------------
 spice-qemu-char.c           | 26 ++++---------
 stubs/qemu-chr-open-spice.c |  2 +-
 4 files changed, 57 insertions(+), 66 deletions(-)

-- 
1.7.12.4

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

[zhanghailiang]

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 <zhang.zhanghailiang@huawei.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 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;
-    }
-
     snprintf(filename_in, CHR_MAX_FILENAME_SIZE, "%s.in", filename);
     snprintf(filename_out, CHR_MAX_FILENAME_SIZE, "%s.out", filename);
     TFR(fd_in = qemu_open(filename_in, O_RDWR | O_BINARY));
@@ -3419,7 +3414,7 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend,
 {
     const char *path = qemu_opt_get(opts, "path");
 
-    if (path == NULL) {
+    if (path == NULL || !path[0]) {
         error_setg(errp, "chardev: file: no filename given");
         return;
     }
@@ -3453,7 +3448,7 @@ static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
 {
     const char *device = qemu_opt_get(opts, "path");
 
-    if (device == NULL) {
+    if (device == NULL || !device[0]) {
         error_setg(errp, "chardev: parallel: no device path given");
         return;
     }
@@ -3466,7 +3461,7 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
 {
     const char *device = qemu_opt_get(opts, "path");
 
-    if (device == NULL) {
+    if (device == NULL || !device[0]) {
         error_setg(errp, "chardev: pipe: no device path given");
         return;
     }
@@ -3515,11 +3510,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     SocketAddress *addr;
 
     if (!path) {
-        if (!host) {
+        if (!host || !host[0]) {
             error_setg(errp, "chardev: socket: no host given");
             return;
         }
-        if (!port) {
+        if (!port || !port[0]) {
             error_setg(errp, "chardev: socket: no port given");
             return;
         }
-- 
1.7.12.4

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

[Alex Bennée]

zhanghailiang <zhang.zhanghailiang@huawei.com> 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 <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  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?

At a minimum I'd replace it with a g_assert(filename) to make the
calling contract clear.

>      snprintf(filename_in, CHR_MAX_FILENAME_SIZE, "%s.in", filename);
>      snprintf(filename_out, CHR_MAX_FILENAME_SIZE, "%s.out",
>      filename);

We'll probably end up with "(null).in" as the filename which may be
exploitation vector.

>      TFR(fd_in = qemu_open(filename_in, O_RDWR | O_BINARY));
> @@ -3419,7 +3414,7 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *path = qemu_opt_get(opts, "path");
>  
> -    if (path == NULL) {
> +    if (path == NULL || !path[0]) {
>          error_setg(errp, "chardev: file: no filename given");
>          return;
>      }
> @@ -3453,7 +3448,7 @@ static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *device = qemu_opt_get(opts, "path");
>  
> -    if (device == NULL) {
> +    if (device == NULL || !device[0]) {
>          error_setg(errp, "chardev: parallel: no device path given");
>          return;
>      }
> @@ -3466,7 +3461,7 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *device = qemu_opt_get(opts, "path");
>  
> -    if (device == NULL) {
> +    if (device == NULL || !device[0]) {
>          error_setg(errp, "chardev: pipe: no device path given");
>          return;
>      }
> @@ -3515,11 +3510,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      SocketAddress *addr;
>  
>      if (!path) {
> -        if (!host) {
> +        if (!host || !host[0]) {
>              error_setg(errp, "chardev: socket: no host given");
>              return;
>          }
> -        if (!port) {
> +        if (!port || !port[0]) {
>              error_setg(errp, "chardev: socket: no port given");
>              return;
>          }

All this boilerplate checking makes me think that either the qemu_opt
machinery should be ensuring we get a valid option string?

-- 
Alex Bennée

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

[Michael Tokarev]

04.11.2014 16:25, Alex Bennée wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> 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 <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>> ---
>>  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?

Yes, the code first calls parse_pipe() and only after it is
successfully completed, it calls open_pipe().  I don't see
a good reason for having assert here.

> 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

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 <zhang.zhanghailiang@huawei.com> 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 <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>> ---
>>>   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
>
> .
>

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

[Alex Bennée]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> On 2014/11/5 15:05, Michael Tokarev wrote:
>> 04.11.2014 16:25, Alex Bennée wrote:
>>> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
<snip>
>> 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.

assert isn't really about informing the user, it just makes an explicit
statement that this API will always have a filled in filename and if it
ever gets a NULL that's a programming bug in using the API, internal or
otherwise.

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

Yes, I wasn't intending to suggest expanding this patch set to encompase
the larger task ;-)

-- 
Alex Bennée

[Qemu-devel] [PATCH v3 2/5] spice-qemu-char: fix parameter checks in qemu_chr_parse_* functions

[zhanghailiang]

For functions qemu_chr_parse_spice_vmc and qemu_chr_parse_spice_port,
we should also check if parameter name is empty, and it will help finding
the wrong configure, such as
'qemu-system-x86_64 -chardev spiceport(or spiceport),id=id,name='

After check the parameter in parse function, we can remove the
superfluous parameter checks in qemu_chr_open_spice_*

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 spice-qemu-char.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8106e06..d3c1f5c 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -290,11 +290,6 @@ CharDriverState *qemu_chr_open_spice_vmc(const char *type)
 {
     const char **psubtype = spice_server_char_device_recognized_subtypes();
 
-    if (type == NULL) {
-        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
-        print_allowed_subtypes();
-        return NULL;
-    }
     for (; *psubtype != NULL; ++psubtype) {
         if (strcmp(type, *psubtype) == 0) {
             break;
@@ -315,11 +310,6 @@ CharDriverState *qemu_chr_open_spice_port(const char *name)
     CharDriverState *chr;
     SpiceCharDriver *s;
 
-    if (name == NULL) {
-        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
-        return NULL;
-    }
-
     chr = chr_open("port", spice_port_set_fe_open);
     s = chr->opaque;
     s->sin.portname = g_strdup(name);
@@ -345,7 +335,7 @@ static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
 {
     const char *name = qemu_opt_get(opts, "name");
 
-    if (name == NULL) {
+    if (name == NULL || !name[0]) {
         error_setg(errp, "chardev: spice channel: no name given");
         return;
     }
@@ -358,7 +348,7 @@ static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
 {
     const char *name = qemu_opt_get(opts, "name");
 
-    if (name == NULL) {
+    if (name == NULL || !name[0]) {
         error_setg(errp, "chardev: spice port: no name given");
         return;
     }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 2/5] spice-qemu-char: fix parameter checks in qemu_chr_parse_* functions

[Alex Bennée]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> For functions qemu_chr_parse_spice_vmc and qemu_chr_parse_spice_port,
> we should also check if parameter name is empty, and it will help finding
> the wrong configure, such as
> 'qemu-system-x86_64 -chardev spiceport(or spiceport),id=id,name='
>
> After check the parameter in parse function, we can remove the
> superfluous parameter checks in qemu_chr_open_spice_*
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  spice-qemu-char.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 8106e06..d3c1f5c 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -290,11 +290,6 @@ CharDriverState *qemu_chr_open_spice_vmc(const char *type)
>  {
>      const char **psubtype = spice_server_char_device_recognized_subtypes();
>  
> -    if (type == NULL) {
> -        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> -        print_allowed_subtypes();
> -        return NULL;
> -    }

As with patch 1 I'd at least assert the contract. But this also drops a
helpful info dump on a missing parameter.

>      for (; *psubtype != NULL; ++psubtype) {
>          if (strcmp(type, *psubtype) == 0) {
>              break;
> @@ -315,11 +310,6 @@ CharDriverState *qemu_chr_open_spice_port(const char *name)
>      CharDriverState *chr;
>      SpiceCharDriver *s;
>  
> -    if (name == NULL) {
> -        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> -        return NULL;
> -    }
> -
>      chr = chr_open("port", spice_port_set_fe_open);
>      s = chr->opaque;
>      s->sin.portname = g_strdup(name);
> @@ -345,7 +335,7 @@ static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *name = qemu_opt_get(opts, "name");
>  
> -    if (name == NULL) {
> +    if (name == NULL || !name[0]) {
>          error_setg(errp, "chardev: spice channel: no name given");
>          return;
>      }
> @@ -358,7 +348,7 @@ static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *name = qemu_opt_get(opts, "name");
>  
> -    if (name == NULL) {
> +    if (name == NULL || !name[0]) {
>          error_setg(errp, "chardev: spice port: no name given");
>          return;
>      }

Same comments as for 1/5

-- 
Alex Bennée

[Qemu-devel] [PATCH v3 3/5] qemu-char: fix incorrect state in error message

[zhanghailiang]

It is either "Failed _to_ do something", or "something failed",
but not "failed something".

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qemu-char.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index a09bbf6..0f38cdd 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1872,25 +1872,25 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
 
     s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hsend) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        fprintf(stderr, "CreateEvent failed\n");
         goto fail;
     }
     s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hrecv) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        fprintf(stderr, "CreateEvent failed\n");
         goto fail;
     }
 
     s->hcom = CreateFile(filename, GENERIC_READ|GENERIC_WRITE, 0, NULL,
                       OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0);
     if (s->hcom == INVALID_HANDLE_VALUE) {
-        fprintf(stderr, "Failed CreateFile (%lu)\n", GetLastError());
+        fprintf(stderr, "CreateFile (%lu) failed\n", GetLastError());
         s->hcom = NULL;
         goto fail;
     }
 
     if (!SetupComm(s->hcom, NRECVBUF, NSENDBUF)) {
-        fprintf(stderr, "Failed SetupComm\n");
+        fprintf(stderr, "SetupComm failed\n");
         goto fail;
     }
 
@@ -1901,23 +1901,23 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
     CommConfigDialog(filename, NULL, &comcfg);
 
     if (!SetCommState(s->hcom, &comcfg.dcb)) {
-        fprintf(stderr, "Failed SetCommState\n");
+        fprintf(stderr, "SetCommState failed\n");
         goto fail;
     }
 
     if (!SetCommMask(s->hcom, EV_ERR)) {
-        fprintf(stderr, "Failed SetCommMask\n");
+        fprintf(stderr, "SetCommMask failed\n");
         goto fail;
     }
 
     cto.ReadIntervalTimeout = MAXDWORD;
     if (!SetCommTimeouts(s->hcom, &cto)) {
-        fprintf(stderr, "Failed SetCommTimeouts\n");
+        fprintf(stderr, "SetCommTimeouts failed\n");
         goto fail;
     }
 
     if (!ClearCommError(s->hcom, &err, &comstat)) {
-        fprintf(stderr, "Failed ClearCommError\n");
+        fprintf(stderr, "ClearCommError failed\n");
         goto fail;
     }
     qemu_add_polling_cb(win_chr_poll, chr);
@@ -2069,12 +2069,12 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
 
     s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hsend) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        fprintf(stderr, "CreateEvent failed\n");
         goto fail;
     }
     s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hrecv) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        fprintf(stderr, "CreateEvent failed\n");
         goto fail;
     }
 
@@ -2084,7 +2084,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
                               PIPE_WAIT,
                               MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, NULL);
     if (s->hcom == INVALID_HANDLE_VALUE) {
-        fprintf(stderr, "Failed CreateNamedPipe (%lu)\n", GetLastError());
+        fprintf(stderr, "CreateNamedPipe (%lu) failed\n", GetLastError());
         s->hcom = NULL;
         goto fail;
     }
@@ -2093,13 +2093,13 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
     ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
     ret = ConnectNamedPipe(s->hcom, &ov);
     if (ret) {
-        fprintf(stderr, "Failed ConnectNamedPipe\n");
+        fprintf(stderr, "ConnectNamedPipe failed\n");
         goto fail;
     }
 
     ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
     if (!ret) {
-        fprintf(stderr, "Failed GetOverlappedResult\n");
+        fprintf(stderr, "GetOverlappedResult failed\n");
         if (ov.hEvent) {
             CloseHandle(ov.hEvent);
             ov.hEvent = NULL;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 3/5] qemu-char: fix incorrect state in error message

[Alex Bennée]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> It is either "Failed _to_ do something", or "something failed",
> but not "failed something".
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Semantically I'm fine to add:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

I'll leave it to the language pedants to rule on the grammar ;-)

> ---
>  qemu-char.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index a09bbf6..0f38cdd 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1872,25 +1872,25 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
>  
>      s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hsend) {
> -        fprintf(stderr, "Failed CreateEvent\n");
> +        fprintf(stderr, "CreateEvent failed\n");
>          goto fail;
>      }
>      s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hrecv) {
> -        fprintf(stderr, "Failed CreateEvent\n");
> +        fprintf(stderr, "CreateEvent failed\n");
>          goto fail;
>      }
>  
>      s->hcom = CreateFile(filename, GENERIC_READ|GENERIC_WRITE, 0, NULL,
>                        OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0);
>      if (s->hcom == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "Failed CreateFile (%lu)\n", GetLastError());
> +        fprintf(stderr, "CreateFile (%lu) failed\n", GetLastError());
>          s->hcom = NULL;
>          goto fail;
>      }
>  
>      if (!SetupComm(s->hcom, NRECVBUF, NSENDBUF)) {
> -        fprintf(stderr, "Failed SetupComm\n");
> +        fprintf(stderr, "SetupComm failed\n");
>          goto fail;
>      }
>  
> @@ -1901,23 +1901,23 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
>      CommConfigDialog(filename, NULL, &comcfg);
>  
>      if (!SetCommState(s->hcom, &comcfg.dcb)) {
> -        fprintf(stderr, "Failed SetCommState\n");
> +        fprintf(stderr, "SetCommState failed\n");
>          goto fail;
>      }
>  
>      if (!SetCommMask(s->hcom, EV_ERR)) {
> -        fprintf(stderr, "Failed SetCommMask\n");
> +        fprintf(stderr, "SetCommMask failed\n");
>          goto fail;
>      }
>  
>      cto.ReadIntervalTimeout = MAXDWORD;
>      if (!SetCommTimeouts(s->hcom, &cto)) {
> -        fprintf(stderr, "Failed SetCommTimeouts\n");
> +        fprintf(stderr, "SetCommTimeouts failed\n");
>          goto fail;
>      }
>  
>      if (!ClearCommError(s->hcom, &err, &comstat)) {
> -        fprintf(stderr, "Failed ClearCommError\n");
> +        fprintf(stderr, "ClearCommError failed\n");
>          goto fail;
>      }
>      qemu_add_polling_cb(win_chr_poll, chr);
> @@ -2069,12 +2069,12 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>  
>      s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hsend) {
> -        fprintf(stderr, "Failed CreateEvent\n");
> +        fprintf(stderr, "CreateEvent failed\n");
>          goto fail;
>      }
>      s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hrecv) {
> -        fprintf(stderr, "Failed CreateEvent\n");
> +        fprintf(stderr, "CreateEvent failed\n");
>          goto fail;
>      }
>  
> @@ -2084,7 +2084,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>                                PIPE_WAIT,
>                                MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, NULL);
>      if (s->hcom == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "Failed CreateNamedPipe (%lu)\n", GetLastError());
> +        fprintf(stderr, "CreateNamedPipe (%lu) failed\n", GetLastError());
>          s->hcom = NULL;
>          goto fail;
>      }
> @@ -2093,13 +2093,13 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>      ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
>      ret = ConnectNamedPipe(s->hcom, &ov);
>      if (ret) {
> -        fprintf(stderr, "Failed ConnectNamedPipe\n");
> +        fprintf(stderr, "ConnectNamedPipe failed\n");
>          goto fail;
>      }
>  
>      ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
>      if (!ret) {
> -        fprintf(stderr, "Failed GetOverlappedResult\n");
> +        fprintf(stderr, "GetOverlappedResult failed\n");
>          if (ov.hEvent) {
>              CloseHandle(ov.hEvent);
>              ov.hEvent = NULL;

-- 
Alex Bennée

Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 3/5] qemu-char: fix incorrect state in error message

[Michael Tokarev]

04.11.2014 13:50, zhanghailiang wrote
> It is either "Failed _to_ do something", or "something failed",
> but not "failed something".

Um.  Heh.  I was afraid of seeing this ;)

My comment about bad grammar was in context of changing error
reporting in this area -- so while chaning error API in use,
it might be a good idea to reword the obviously wrong error
messages _too_, on the same line..

I'll fold this to the next patch, so not to resend whole thing
again.  Oh well ;)

The error messages aren't nice still, but maybe it will be
more clean and easier to understand from the context provided
by the error api.

Thanks,

/mjt

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

[zhanghailiang]

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];
@@ -1140,12 +1140,12 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr)
     fd_chr_close(chr);
 }
 
-static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
+static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts, Error **errp)
 {
     CharDriverState *chr;
 
     if (is_daemonized()) {
-        error_report("cannot use stdio with -daemonize");
+        error_setg(errp, "cannot use stdio with -daemonize");
         return NULL;
     }
 
@@ -1861,7 +1861,8 @@ static void win_chr_close(CharDriverState *chr)
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-static int win_chr_init(CharDriverState *chr, const char *filename)
+static void win_chr_init(CharDriverState *chr, const char *filename,
+                         Error **errp)
 {
     WinCharState *s = chr->opaque;
     COMMCONFIG comcfg;
@@ -1872,25 +1873,25 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
 
     s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hsend) {
-        fprintf(stderr, "CreateEvent failed\n");
+        error_setg(errp, "CreateEvent failed");
         goto fail;
     }
     s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hrecv) {
-        fprintf(stderr, "CreateEvent failed\n");
+        error_setg(errp, "CreateEvent failed");
         goto fail;
     }
 
     s->hcom = CreateFile(filename, GENERIC_READ|GENERIC_WRITE, 0, NULL,
                       OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0);
     if (s->hcom == INVALID_HANDLE_VALUE) {
-        fprintf(stderr, "CreateFile (%lu) failed\n", GetLastError());
+        error_setg(errp, "CreateFile (%lu) failed", GetLastError());
         s->hcom = NULL;
         goto fail;
     }
 
     if (!SetupComm(s->hcom, NRECVBUF, NSENDBUF)) {
-        fprintf(stderr, "SetupComm failed\n");
+        error_setg(errp, "SetupComm failed");
         goto fail;
     }
 
@@ -1901,31 +1902,30 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
     CommConfigDialog(filename, NULL, &comcfg);
 
     if (!SetCommState(s->hcom, &comcfg.dcb)) {
-        fprintf(stderr, "SetCommState failed\n");
+        error_setg(errp, "SetCommState failed");
         goto fail;
     }
 
     if (!SetCommMask(s->hcom, EV_ERR)) {
-        fprintf(stderr, "SetCommMask failed\n");
+        error_setg(errp, "SetCommMask failed");
         goto fail;
     }
 
     cto.ReadIntervalTimeout = MAXDWORD;
     if (!SetCommTimeouts(s->hcom, &cto)) {
-        fprintf(stderr, "SetCommTimeouts failed\n");
+        error_setg(errp, "SetCommTimeouts failed");
         goto fail;
     }
 
     if (!ClearCommError(s->hcom, &err, &comstat)) {
-        fprintf(stderr, "ClearCommError failed\n");
+        error_setg(errp, "ClearCommError failed");
         goto fail;
     }
     qemu_add_polling_cb(win_chr_poll, chr);
-    return 0;
+    return;
 
  fail:
     win_chr_close(chr);
-    return -1;
 }
 
 /* Called with chr_write_lock held.  */
@@ -2022,10 +2022,12 @@ static int win_chr_poll(void *opaque)
     return 0;
 }
 
-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;
     }
     return chr;
@@ -2057,11 +2061,11 @@ static int win_chr_pipe_poll(void *opaque)
     return 0;
 }
 
-static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
+static void win_chr_pipe_init(CharDriverState *chr, const char *filename,
+                              Error **errp)
 {
     WinCharState *s = chr->opaque;
     OVERLAPPED ov;
-    int ret;
     DWORD size;
     char openname[CHR_MAX_FILENAME_SIZE];
 
@@ -2069,12 +2073,12 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
 
     s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hsend) {
-        fprintf(stderr, "CreateEvent failed\n");
+        error_setg(errp, "CreateEvent failed");
         goto fail;
     }
     s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hrecv) {
-        fprintf(stderr, "CreateEvent failed\n");
+        error_setg(errp, "CreateEvent failed");
         goto fail;
     }
 
@@ -2084,7 +2088,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
                               PIPE_WAIT,
                               MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, NULL);
     if (s->hcom == INVALID_HANDLE_VALUE) {
-        fprintf(stderr, "CreateNamedPipe (%lu) failed\n", GetLastError());
+        error_setg(errp, "CreateNamedPipe (%lu) failed", GetLastError());
         s->hcom = NULL;
         goto fail;
     }
@@ -2093,13 +2097,13 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
     ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
     ret = ConnectNamedPipe(s->hcom, &ov);
     if (ret) {
-        fprintf(stderr, "ConnectNamedPipe failed\n");
+        error_setg(errp, "ConnectNamedPipe failed");
         goto fail;
     }
 
     ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
     if (!ret) {
-        fprintf(stderr, "GetOverlappedResult failed\n");
+        error_setg(errp, "GetOverlappedResult failed");
         if (ov.hEvent) {
             CloseHandle(ov.hEvent);
             ov.hEvent = NULL;
@@ -2112,19 +2116,19 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
         ov.hEvent = NULL;
     }
     qemu_add_polling_cb(win_chr_pipe_poll, chr);
-    return 0;
+    return;
 
  fail:
     win_chr_close(chr);
-    return -1;
 }
 
 
-static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
+static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts, Error **errp)
 {
     const char *filename = opts->device;
     CharDriverState *chr;
     WinCharState *s;
+    Error *local_err = NULL;
 
     chr = qemu_chr_alloc();
     s = g_malloc0(sizeof(WinCharState));
@@ -2132,9 +2136,11 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
     chr->chr_write = win_chr_write;
     chr->chr_close = win_chr_close;
 
-    if (win_chr_pipe_init(chr, filename) < 0) {
+    win_chr_pipe_init(chr, filename, &local_err);
+    if (local_err) {
         g_free(s);
         g_free(chr);
+        error_propagate(errp, local_err);
         return NULL;
     }
     return chr;
@@ -2294,7 +2300,7 @@ static void win_stdio_close(CharDriverState *chr)
     g_free(chr);
 }
 
-static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
+static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts, Error **errp)
 {
     CharDriverState   *chr;
     WinStdioCharState *stdio;
@@ -2306,7 +2312,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
 
     stdio->hStdIn = GetStdHandle(STD_INPUT_HANDLE);
     if (stdio->hStdIn == INVALID_HANDLE_VALUE) {
-        fprintf(stderr, "cannot open stdio: invalid handle\n");
+        error_report(errp, "cannot open stdio: invalid handle");
         exit(1);
     }
 
@@ -2319,7 +2325,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
     if (is_console) {
         if (qemu_add_wait_object(stdio->hStdIn,
                                  win_stdio_wait_func, chr)) {
-            fprintf(stderr, "qemu_add_wait_object: failed\n");
+            error_setg(errp, "qemu_add_wait_object: failed");
         }
     } else {
         DWORD   dwId;
@@ -2332,12 +2338,12 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
         if (stdio->hInputThread == INVALID_HANDLE_VALUE
             || stdio->hInputReadyEvent == INVALID_HANDLE_VALUE
             || stdio->hInputDoneEvent == INVALID_HANDLE_VALUE) {
-            fprintf(stderr, "cannot create stdio thread or event\n");
+            error_report(errp, "cannot create stdio thread or event");
             exit(1);
         }
         if (qemu_add_wait_object(stdio->hInputReadyEvent,
                                  win_stdio_thread_wait_func, chr)) {
-            fprintf(stderr, "qemu_add_wait_object: failed\n");
+            error_setg(errp, "qemu_add_wait_object: failed");
         }
     }
 
@@ -3994,7 +4000,7 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
 static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
                                                 Error **errp)
 {
-    return qemu_chr_open_win_path(serial->device);
+    return qemu_chr_open_win_path(serial->device, errp);
 }
 
 static CharDriverState *qmp_chardev_open_parallel(ChardevHostdev *parallel,
@@ -4204,7 +4210,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         chr = qmp_chardev_open_parallel(backend->parallel, errp);
         break;
     case CHARDEV_BACKEND_KIND_PIPE:
-        chr = qemu_chr_open_pipe(backend->pipe);
+        chr = qemu_chr_open_pipe(backend->pipe, errp);
         break;
     case CHARDEV_BACKEND_KIND_SOCKET:
         chr = qmp_chardev_open_socket(backend->socket, errp);
@@ -4241,7 +4247,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         chr = chr_testdev_init();
         break;
     case CHARDEV_BACKEND_KIND_STDIO:
-        chr = qemu_chr_open_stdio(backend->stdio);
+        chr = qemu_chr_open_stdio(backend->stdio, errp);
         break;
 #ifdef _WIN32
     case CHARDEV_BACKEND_KIND_CONSOLE:
-- 
1.7.12.4

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

[Alex Bennée]

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?

>  
> -static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
> +static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts, Error **errp)
>  {
>      CharDriverState *chr;
>  
>      if (is_daemonized()) {
> -        error_report("cannot use stdio with -daemonize");
> +        error_setg(errp, "cannot use stdio with -daemonize");
>          return NULL;
>      }
>  
> @@ -1861,7 +1861,8 @@ static void win_chr_close(CharDriverState *chr)
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
>  
> -static int win_chr_init(CharDriverState *chr, const char *filename)
> +static void win_chr_init(CharDriverState *chr, const char *filename,
> +                         Error **errp)
>  {
>      WinCharState *s = chr->opaque;
>      COMMCONFIG comcfg;
> @@ -1872,25 +1873,25 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
>  
>      s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hsend) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>      s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hrecv) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>  
>      s->hcom = CreateFile(filename, GENERIC_READ|GENERIC_WRITE, 0, NULL,
>                        OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0);
>      if (s->hcom == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "CreateFile (%lu) failed\n", GetLastError());
> +        error_setg(errp, "CreateFile (%lu) failed", GetLastError());
>          s->hcom = NULL;
>          goto fail;
>      }
>  
>      if (!SetupComm(s->hcom, NRECVBUF, NSENDBUF)) {
> -        fprintf(stderr, "SetupComm failed\n");
> +        error_setg(errp, "SetupComm failed");
>          goto fail;
>      }
>  
> @@ -1901,31 +1902,30 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
>      CommConfigDialog(filename, NULL, &comcfg);
>  
>      if (!SetCommState(s->hcom, &comcfg.dcb)) {
> -        fprintf(stderr, "SetCommState failed\n");
> +        error_setg(errp, "SetCommState failed");
>          goto fail;
>      }
>  
>      if (!SetCommMask(s->hcom, EV_ERR)) {
> -        fprintf(stderr, "SetCommMask failed\n");
> +        error_setg(errp, "SetCommMask failed");
>          goto fail;
>      }
>  
>      cto.ReadIntervalTimeout = MAXDWORD;
>      if (!SetCommTimeouts(s->hcom, &cto)) {
> -        fprintf(stderr, "SetCommTimeouts failed\n");
> +        error_setg(errp, "SetCommTimeouts failed");
>          goto fail;
>      }
>  
>      if (!ClearCommError(s->hcom, &err, &comstat)) {
> -        fprintf(stderr, "ClearCommError failed\n");
> +        error_setg(errp, "ClearCommError failed");
>          goto fail;
>      }
>      qemu_add_polling_cb(win_chr_poll, chr);
> -    return 0;
> +    return;
>  
>   fail:
>      win_chr_close(chr);
> -    return -1;
>  }
>  
>  /* Called with chr_write_lock held.  */
> @@ -2022,10 +2022,12 @@ static int win_chr_poll(void *opaque)
>      return 0;
>  }
>  
> -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.

>      }
>      return chr;
> @@ -2057,11 +2061,11 @@ static int win_chr_pipe_poll(void *opaque)
>      return 0;
>  }
>  
> -static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
> +static void win_chr_pipe_init(CharDriverState *chr, const char *filename,
> +                              Error **errp)
>  {
>      WinCharState *s = chr->opaque;
>      OVERLAPPED ov;
> -    int ret;
>      DWORD size;
>      char openname[CHR_MAX_FILENAME_SIZE];
>  
> @@ -2069,12 +2073,12 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>  
>      s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hsend) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>      s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
>      if (!s->hrecv) {
> -        fprintf(stderr, "CreateEvent failed\n");
> +        error_setg(errp, "CreateEvent failed");
>          goto fail;
>      }
>  
> @@ -2084,7 +2088,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>                                PIPE_WAIT,
>                                MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, NULL);
>      if (s->hcom == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "CreateNamedPipe (%lu) failed\n", GetLastError());
> +        error_setg(errp, "CreateNamedPipe (%lu) failed", GetLastError());
>          s->hcom = NULL;
>          goto fail;
>      }
> @@ -2093,13 +2097,13 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>      ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
>      ret = ConnectNamedPipe(s->hcom, &ov);
>      if (ret) {
> -        fprintf(stderr, "ConnectNamedPipe failed\n");
> +        error_setg(errp, "ConnectNamedPipe failed");
>          goto fail;
>      }
>  
>      ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
>      if (!ret) {
> -        fprintf(stderr, "GetOverlappedResult failed\n");
> +        error_setg(errp, "GetOverlappedResult failed");
>          if (ov.hEvent) {
>              CloseHandle(ov.hEvent);
>              ov.hEvent = NULL;
> @@ -2112,19 +2116,19 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
>          ov.hEvent = NULL;
>      }
>      qemu_add_polling_cb(win_chr_pipe_poll, chr);
> -    return 0;
> +    return;
>  
>   fail:
>      win_chr_close(chr);
> -    return -1;
>  }
>  
>  
> -static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
> +static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts, Error **errp)
>  {
>      const char *filename = opts->device;
>      CharDriverState *chr;
>      WinCharState *s;
> +    Error *local_err = NULL;
>  
>      chr = qemu_chr_alloc();
>      s = g_malloc0(sizeof(WinCharState));
> @@ -2132,9 +2136,11 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
>      chr->chr_write = win_chr_write;
>      chr->chr_close = win_chr_close;
>  
> -    if (win_chr_pipe_init(chr, filename) < 0) {
> +    win_chr_pipe_init(chr, filename, &local_err);
> +    if (local_err) {
>          g_free(s);
>          g_free(chr);
> +        error_propagate(errp, local_err);
>          return NULL;
>      }
>      return chr;
> @@ -2294,7 +2300,7 @@ static void win_stdio_close(CharDriverState *chr)
>      g_free(chr);
>  }

Same comments as above.

>  
> -static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
> +static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts, Error **errp)
>  {
>      CharDriverState   *chr;
>      WinStdioCharState *stdio;
> @@ -2306,7 +2312,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>  
>      stdio->hStdIn = GetStdHandle(STD_INPUT_HANDLE);
>      if (stdio->hStdIn == INVALID_HANDLE_VALUE) {
> -        fprintf(stderr, "cannot open stdio: invalid handle\n");
> +        error_report(errp, "cannot open stdio: invalid handle");
>          exit(1);
>      }
>  
> @@ -2319,7 +2325,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>      if (is_console) {
>          if (qemu_add_wait_object(stdio->hStdIn,
>                                   win_stdio_wait_func, chr)) {
> -            fprintf(stderr, "qemu_add_wait_object: failed\n");
> +            error_setg(errp, "qemu_add_wait_object: failed");
>          }
>      } else {
>          DWORD   dwId;
> @@ -2332,12 +2338,12 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>          if (stdio->hInputThread == INVALID_HANDLE_VALUE
>              || stdio->hInputReadyEvent == INVALID_HANDLE_VALUE
>              || stdio->hInputDoneEvent == INVALID_HANDLE_VALUE) {
> -            fprintf(stderr, "cannot create stdio thread or event\n");
> +            error_report(errp, "cannot create stdio thread or event");
>              exit(1);
>          }
>          if (qemu_add_wait_object(stdio->hInputReadyEvent,
>                                   win_stdio_thread_wait_func, chr)) {
> -            fprintf(stderr, "qemu_add_wait_object: failed\n");
> +            error_setg(errp, "qemu_add_wait_object: failed");
>          }
>      }
>  
> @@ -3994,7 +4000,7 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
>  static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
>                                                  Error **errp)
>  {
> -    return qemu_chr_open_win_path(serial->device);
> +    return qemu_chr_open_win_path(serial->device, errp);
>  }
>  
>  static CharDriverState *qmp_chardev_open_parallel(ChardevHostdev *parallel,
> @@ -4204,7 +4210,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
>          chr = qmp_chardev_open_parallel(backend->parallel, errp);
>          break;
>      case CHARDEV_BACKEND_KIND_PIPE:
> -        chr = qemu_chr_open_pipe(backend->pipe);
> +        chr = qemu_chr_open_pipe(backend->pipe, errp);
>          break;
>      case CHARDEV_BACKEND_KIND_SOCKET:
>          chr = qmp_chardev_open_socket(backend->socket, errp);
> @@ -4241,7 +4247,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
>          chr = chr_testdev_init();
>          break;
>      case CHARDEV_BACKEND_KIND_STDIO:
> -        chr = qemu_chr_open_stdio(backend->stdio);
> +        chr = qemu_chr_open_stdio(backend->stdio, errp);
>          break;
>  #ifdef _WIN32
>      case CHARDEV_BACKEND_KIND_CONSOLE:

-- 
Alex Bennée

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

[Michael Tokarev]

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

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

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

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

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.

[...]

[Qemu-devel] [PATCH v3 5/5] spice-qemu-char: convert some functions to use Error API

[zhanghailiang]

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 include/ui/qemu-spice.h     |  2 +-
 qemu-char.c                 |  2 +-
 spice-qemu-char.c           | 12 ++++++------
 stubs/qemu-chr-open-spice.c |  2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index a93b4b2..33882aa 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -48,7 +48,7 @@ int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
 void do_info_spice_print(Monitor *mon, const QObject *data);
 void do_info_spice(Monitor *mon, QObject **ret_data);
 
-CharDriverState *qemu_chr_open_spice_vmc(const char *type);
+CharDriverState *qemu_chr_open_spice_vmc(const char *type, Error **errp);
 #if SPICE_SERVER_VERSION >= 0x000c02
 CharDriverState *qemu_chr_open_spice_port(const char *name);
 void qemu_spice_register_ports(void);
diff --git a/qemu-char.c b/qemu-char.c
index a1d25c7..df24c65 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4256,7 +4256,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
 #endif
 #ifdef CONFIG_SPICE
     case CHARDEV_BACKEND_KIND_SPICEVMC:
-        chr = qemu_chr_open_spice_vmc(backend->spicevmc->type);
+        chr = qemu_chr_open_spice_vmc(backend->spicevmc->type, errp);
         break;
     case CHARDEV_BACKEND_KIND_SPICEPORT:
         chr = qemu_chr_open_spice_port(backend->spiceport->fqdn);
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index d3c1f5c..721c895 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -249,16 +249,16 @@ static void print_allowed_subtypes(void)
     const char** psubtype;
     int i;
 
-    fprintf(stderr, "allowed names: ");
+    error_printf("allowed names: ");
     for(i=0, psubtype = spice_server_char_device_recognized_subtypes();
         *psubtype != NULL; ++psubtype, ++i) {
         if (i == 0) {
-            fprintf(stderr, "%s", *psubtype);
+            error_printf("%s", *psubtype);
         } else {
-            fprintf(stderr, ", %s", *psubtype);
+            error_printf(", %s", *psubtype);
         }
     }
-    fprintf(stderr, "\n");
+    error_printf("\n");
 }
 
 static CharDriverState *chr_open(const char *subtype,
@@ -286,7 +286,7 @@ static CharDriverState *chr_open(const char *subtype,
     return chr;
 }
 
-CharDriverState *qemu_chr_open_spice_vmc(const char *type)
+CharDriverState *qemu_chr_open_spice_vmc(const char *type, Error **errp)
 {
     const char **psubtype = spice_server_char_device_recognized_subtypes();
 
@@ -296,7 +296,7 @@ CharDriverState *qemu_chr_open_spice_vmc(const char *type)
         }
     }
     if (*psubtype == NULL) {
-        fprintf(stderr, "spice-qemu-char: unsupported type: %s\n", type);
+        error_setg(errp, "spice-qemu-char: unsupported type: %s", type);
         print_allowed_subtypes();
         return NULL;
     }
diff --git a/stubs/qemu-chr-open-spice.c b/stubs/qemu-chr-open-spice.c
index f1c4849..55826ed 100644
--- a/stubs/qemu-chr-open-spice.c
+++ b/stubs/qemu-chr-open-spice.c
@@ -1,7 +1,7 @@
 #include "qemu-common.h"
 #include "ui/qemu-spice.h"
 
-CharDriverState *qemu_chr_open_spice_vmc(const char *type)
+CharDriverState *qemu_chr_open_spice_vmc(const char *type, Error **errp)
 {
     return NULL;
 }
-- 
1.7.12.4

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

[Alex Bennée]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
<snip>
>  
> -CharDriverState *qemu_chr_open_spice_vmc(const char *type)
> +CharDriverState *qemu_chr_open_spice_vmc(const char *type, Error **errp)
>  {
<snip>
> diff --git a/stubs/qemu-chr-open-spice.c b/stubs/qemu-chr-open-spice.c
> index f1c4849..55826ed 100644
> --- a/stubs/qemu-chr-open-spice.c
> +++ b/stubs/qemu-chr-open-spice.c
> @@ -1,7 +1,7 @@
>  #include "qemu-common.h"
>  #include "ui/qemu-spice.h"
>  
> -CharDriverState *qemu_chr_open_spice_vmc(const char *type)
> +CharDriverState *qemu_chr_open_spice_vmc(const char *type, Error **errp)
>  {
>      return NULL;
>  }

Ahh I may have missed it was a stub in the previous patch.


-- 
Alex Bennée