[Qemu-trivial] [PATCH v2 0/5] Trivial patch about qemu-char

[Qemu-trivial] [PATCH v2 0/5] Trivial patch about qemu-char

[zhanghailiang]

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

---
zhanghailiang (5):
  qemu-char: fix parameter check for some qemu_chr_parse_* functions
  qemu-char: remove unnecessary in-parameter check for
    qemu_chr_parse_pipe
  spice-qemu-char: fix parameter check for qemu_chr_parse_* functions
  qemu-char: convert some open functions to use Error API
  spice-qemu-char: convert qemu_chr_open_spice_vmc to use Error API

 include/ui/qemu-spice.h     |  2 +-
 qemu-char.c                 | 65 +++++++++++++++++++++------------------------
 spice-qemu-char.c           | 19 ++++---------
 stubs/qemu-chr-open-spice.c |  2 +-
 4 files changed, 38 insertions(+), 50 deletions(-)

-- 
1.7.12.4

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

[zhanghailiang]

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

---
zhanghailiang (5):
  qemu-char: fix parameter check for some qemu_chr_parse_* functions
  qemu-char: remove unnecessary in-parameter check for
    qemu_chr_parse_pipe
  spice-qemu-char: fix parameter check for qemu_chr_parse_* functions
  qemu-char: convert some open functions to use Error API
  spice-qemu-char: convert qemu_chr_open_spice_vmc to use Error API

 include/ui/qemu-spice.h     |  2 +-
 qemu-char.c                 | 65 +++++++++++++++++++++------------------------
 spice-qemu-char.c           | 19 ++++---------
 stubs/qemu-chr-open-spice.c |  2 +-
 4 files changed, 38 insertions(+), 50 deletions(-)

-- 
1.7.12.4

[Qemu-trivial] [PATCH v2 1/5] qemu-char: fix parameter check for some qemu_chr_parse_* functions

[zhanghailiang]

For some qemu_chr_parse_* functions, we just check whether the parameter
is NULL or not, not check its length.

For example:
qemu-system-x86_64 -chardev pipe,id=id,path=
It will pass the check of NULL, and finds the error until trying
to open it.

So we should find the error by check its length, just like what
qemu_chr_parse_udp does, it will help find what exactly is wrong.

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

diff --git a/qemu-char.c b/qemu-char.c
index bd0709b..04d747a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3419,7 +3419,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 || strlen(path) == 0) {
         error_setg(errp, "chardev: file: no filename given");
         return;
     }
@@ -3453,7 +3453,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 || strlen(device) == 0) {
         error_setg(errp, "chardev: parallel: no device path given");
         return;
     }
@@ -3466,7 +3466,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 || strlen(device) == 0) {
         error_setg(errp, "chardev: pipe: no device path given");
         return;
     }
@@ -3515,11 +3515,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     SocketAddress *addr;
 
     if (!path) {
-        if (!host) {
+        if (!host || strlen(host) == 0) {
             error_setg(errp, "chardev: socket: no host given");
             return;
         }
-        if (!port) {
+        if (!port || strlen(port) == 0) {
             error_setg(errp, "chardev: socket: no port given");
             return;
         }
-- 
1.7.12.4

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

[zhanghailiang]

For some qemu_chr_parse_* functions, we just check whether the parameter
is NULL or not, not check its length.

For example:
qemu-system-x86_64 -chardev pipe,id=id,path=
It will pass the check of NULL, and finds the error until trying
to open it.

So we should find the error by check its length, just like what
qemu_chr_parse_udp does, it will help find what exactly is wrong.

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

diff --git a/qemu-char.c b/qemu-char.c
index bd0709b..04d747a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3419,7 +3419,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 || strlen(path) == 0) {
         error_setg(errp, "chardev: file: no filename given");
         return;
     }
@@ -3453,7 +3453,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 || strlen(device) == 0) {
         error_setg(errp, "chardev: parallel: no device path given");
         return;
     }
@@ -3466,7 +3466,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 || strlen(device) == 0) {
         error_setg(errp, "chardev: pipe: no device path given");
         return;
     }
@@ -3515,11 +3515,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     SocketAddress *addr;
 
     if (!path) {
-        if (!host) {
+        if (!host || strlen(host) == 0) {
             error_setg(errp, "chardev: socket: no host given");
             return;
         }
-        if (!port) {
+        if (!port || strlen(port) == 0) {
             error_setg(errp, "chardev: socket: no port given");
             return;
         }
-- 
1.7.12.4

[Qemu-trivial] [PATCH v2 2/5] qemu-char: remove unnecessary in-parameter check for qemu_chr_parse_pipe

[zhanghailiang]

The check has been done in qemu_chr_parse_pipe, so we don't need to
check it again in qemu_chr_parse_pipe.

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

diff --git a/qemu-char.c b/qemu-char.c
index 04d747a..e1f0e28 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));
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 2/5] qemu-char: remove unnecessary in-parameter check for qemu_chr_parse_pipe

[zhanghailiang]

The check has been done in qemu_chr_parse_pipe, so we don't need to
check it again in qemu_chr_parse_pipe.

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

diff --git a/qemu-char.c b/qemu-char.c
index 04d747a..e1f0e28 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));
-- 
1.7.12.4

[Qemu-trivial] [PATCH v2 3/5] spice-qemu-char: fix parameter checks for qemu_chr_parse_* functions

[zhanghailiang]

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

Also remove the superfluous parameter checks in qemu_chr_open_spice_*

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

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8106e06..48ee37f 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,8 +335,9 @@ 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 || strlen(name) == 0) {
         error_setg(errp, "chardev: spice channel: no name given");
+        print_allowed_subtypes();
         return;
     }
     backend->spicevmc = g_new0(ChardevSpiceChannel, 1);
@@ -358,7 +349,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 || strlen(name) == 0) {
         error_setg(errp, "chardev: spice port: no name given");
         return;
     }
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 3/5] spice-qemu-char: fix parameter checks for qemu_chr_parse_* functions

[zhanghailiang]

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

Also remove the superfluous parameter checks in qemu_chr_open_spice_*

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

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8106e06..48ee37f 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,8 +335,9 @@ 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 || strlen(name) == 0) {
         error_setg(errp, "chardev: spice channel: no name given");
+        print_allowed_subtypes();
         return;
     }
     backend->spicevmc = g_new0(ChardevSpiceChannel, 1);
@@ -358,7 +349,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 || strlen(name) == 0) {
         error_setg(errp, "chardev: spice port: no name given");
         return;
     }
-- 
1.7.12.4

[Qemu-trivial] [PATCH v2 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 | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e1f0e28..3ebdfe7 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;
     }
 
@@ -1388,8 +1388,8 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     ret->pty = g_strdup(pty_name);
     ret->has_pty = true;
 
-    fprintf(stderr, "char device redirected to %s (label %s)\n",
-            pty_name, id);
+    error_report("char device redirected to %s (label %s)",
+                 pty_name, id);
 
     s = g_malloc0(sizeof(PtyCharDriver));
     chr->opaque = s;
@@ -2057,11 +2057,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 +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");
+        error_setg(errp, "Failed CreateEvent");
         goto fail;
     }
     s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hrecv) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        error_setg(errp, "Failed CreateEvent");
         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());
+        error_setg(errp, "Failed CreateNamedPipe (%lu)", 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");
+        error_setg(errp, "Failed ConnectNamedPipe");
         goto fail;
     }
 
     ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
     if (!ret) {
-        fprintf(stderr, "Failed GetOverlappedResult\n");
+        error_setg(errp, "Failed GetOverlappedResult");
         if (ov.hEvent) {
             CloseHandle(ov.hEvent);
             ov.hEvent = NULL;
@@ -2112,19 +2112,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 +2132,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 +2296,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 +2308,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 +2321,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 +2334,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");
         }
     }
 
@@ -4204,7 +4206,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 +4243,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

[Qemu-devel] [PATCH v2 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 | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e1f0e28..3ebdfe7 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;
     }
 
@@ -1388,8 +1388,8 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     ret->pty = g_strdup(pty_name);
     ret->has_pty = true;
 
-    fprintf(stderr, "char device redirected to %s (label %s)\n",
-            pty_name, id);
+    error_report("char device redirected to %s (label %s)",
+                 pty_name, id);
 
     s = g_malloc0(sizeof(PtyCharDriver));
     chr->opaque = s;
@@ -2057,11 +2057,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 +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");
+        error_setg(errp, "Failed CreateEvent");
         goto fail;
     }
     s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
     if (!s->hrecv) {
-        fprintf(stderr, "Failed CreateEvent\n");
+        error_setg(errp, "Failed CreateEvent");
         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());
+        error_setg(errp, "Failed CreateNamedPipe (%lu)", 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");
+        error_setg(errp, "Failed ConnectNamedPipe");
         goto fail;
     }
 
     ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
     if (!ret) {
-        fprintf(stderr, "Failed GetOverlappedResult\n");
+        error_setg(errp, "Failed GetOverlappedResult");
         if (ov.hEvent) {
             CloseHandle(ov.hEvent);
             ov.hEvent = NULL;
@@ -2112,19 +2112,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 +2132,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 +2296,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 +2308,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 +2321,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 +2334,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");
         }
     }
 
@@ -4204,7 +4206,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 +4243,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

[Qemu-trivial] [PATCH v2 5/5] spice-qemu-char: convert qemu_chr_open_spice_vmc 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           | 4 ++--
 stubs/qemu-chr-open-spice.c | 2 +-
 4 files changed, 5 insertions(+), 5 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 3ebdfe7..aa528ae 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4252,7 +4252,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 48ee37f..5c47287 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -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

[Qemu-devel] [PATCH v2 5/5] spice-qemu-char: convert qemu_chr_open_spice_vmc 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           | 4 ++--
 stubs/qemu-chr-open-spice.c | 2 +-
 4 files changed, 5 insertions(+), 5 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 3ebdfe7..aa528ae 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4252,7 +4252,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 48ee37f..5c47287 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -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-trivial] [PATCH v2 5/5] spice-qemu-char: convert qemu_chr_open_spice_vmc to use Error API

[Michael Tokarev]

03.11.2014 12:44, zhanghailiang wrote:
[]
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -4252,7 +4252,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);

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

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v2 5/5] spice-qemu-char: convert qemu_chr_open_spice_vmc to use Error API

[Michael Tokarev]

03.11.2014 12:44, zhanghailiang wrote:
[]
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -4252,7 +4252,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);

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

Thanks,

/mjt

Re: [Qemu-trivial] [PATCH v2 5/5] spice-qemu-char: convert qemu_chr_open_spice_vmc to use Error API

[zhanghailiang]

On 2014/11/3 18:04, Michael Tokarev wrote:
> 03.11.2014 12:44, zhanghailiang wrote:
> []
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -4252,7 +4252,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);
>
> 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.

Re: [Qemu-devel] [PATCH v2 5/5] spice-qemu-char: convert qemu_chr_open_spice_vmc to use Error API

[zhanghailiang]

On 2014/11/3 18:04, Michael Tokarev wrote:
> 03.11.2014 12:44, zhanghailiang wrote:
> []
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -4252,7 +4252,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);
>
> 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.

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

[Michael Tokarev]

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.

Speaking of Error API -- what is the general consensus of this?  We have
TONS of various way to report errors now, and we had several incarnations
of various error APIs too, are we going to use some common API finally,
or is the conversion endless, expecting new APIs to arrive?

And one more thing.  Patch 4 does this:

@@ -1388,8 +1388,8 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     ret->pty = g_strdup(pty_name);
     ret->has_pty = true;

-    fprintf(stderr, "char device redirected to %s (label %s)\n",
-            pty_name, id);
+    error_report("char device redirected to %s (label %s)",
+                 pty_name, id);

     s = g_malloc0(sizeof(PtyCharDriver));
     chr->opaque = s;

This is not really correct.  First it is not really an error, it is
an informational message.  But also, there are many scripts out there
who expects this very format of this message to find the entry to
that char device.  Converting this message to error_report() changes
its format, so scrips will be unable to find the device anymore.

Take a look at history of this place, -- I remember there was a rather
hot discussion when the last part, "(label %)", has been added (initial
message was without this label).  Initial suggestion was to change
wordin to 'char device $LABEL redirected to $DEVICE', but even if it
is much more readable and correct, we agreed to add that "label" part
to the end - not that it preserves original message, but at least it
makes less scripts to fail...

So at least this hunk should not be applied.  I think this place
deserves a comment.

I'm sorry for being so picky, but I think I give enough reasons
explaining why, and these reasons are serious enough.

Thanks,

/mjt

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

[Michael Tokarev]

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.

Speaking of Error API -- what is the general consensus of this?  We have
TONS of various way to report errors now, and we had several incarnations
of various error APIs too, are we going to use some common API finally,
or is the conversion endless, expecting new APIs to arrive?

And one more thing.  Patch 4 does this:

@@ -1388,8 +1388,8 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
     ret->pty = g_strdup(pty_name);
     ret->has_pty = true;

-    fprintf(stderr, "char device redirected to %s (label %s)\n",
-            pty_name, id);
+    error_report("char device redirected to %s (label %s)",
+                 pty_name, id);

     s = g_malloc0(sizeof(PtyCharDriver));
     chr->opaque = s;

This is not really correct.  First it is not really an error, it is
an informational message.  But also, there are many scripts out there
who expects this very format of this message to find the entry to
that char device.  Converting this message to error_report() changes
its format, so scrips will be unable to find the device anymore.

Take a look at history of this place, -- I remember there was a rather
hot discussion when the last part, "(label %)", has been added (initial
message was without this label).  Initial suggestion was to change
wordin to 'char device $LABEL redirected to $DEVICE', but even if it
is much more readable and correct, we agreed to add that "label" part
to the end - not that it preserves original message, but at least it
makes less scripts to fail...

So at least this hunk should not be applied.  I think this place
deserves a comment.

I'm sorry for being so picky, but I think I give enough reasons
explaining why, and these reasons are serious enough.

Thanks,

/mjt

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

[zhanghailiang]

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

> Speaking of Error API -- what is the general consensus of this?  We have
> TONS of various way to report errors now, and we had several incarnations
> of various error APIs too, are we going to use some common API finally,
> or is the conversion endless, expecting new APIs to arrive?
>

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.

> And one more thing.  Patch 4 does this:
>
> @@ -1388,8 +1388,8 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>       ret->pty = g_strdup(pty_name);
>       ret->has_pty = true;
>
> -    fprintf(stderr, "char device redirected to %s (label %s)\n",
> -            pty_name, id);
> +    error_report("char device redirected to %s (label %s)",
> +                 pty_name, id);
>
>       s = g_malloc0(sizeof(PtyCharDriver));
>       chr->opaque = s;
>
> This is not really correct.  First it is not really an error, it is
> an informational message.  But also, there are many scripts out there
> who expects this very format of this message to find the entry to
> that char device.  Converting this message to error_report() changes
> its format, so scrips will be unable to find the device anymore.
>
> Take a look at history of this place, -- I remember there was a rather
> hot discussion when the last part, "(label %)", has been added (initial
> message was without this label).  Initial suggestion was to change
> wordin to 'char device $LABEL redirected to $DEVICE', but even if it
> is much more readable and correct, we agreed to add that "label" part
> to the end - not that it preserves original message, but at least it
> makes less scripts to fail...
>

I got it, thanks very much for your explanation, and your patience;)

> So at least this hunk should not be applied.  I think this place
> deserves a comment.
>
> I'm sorry for being so picky, but I think I give enough reasons
> explaining why, and these reasons are serious enough.

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

Thanks,
zhanghailiang

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

[zhanghailiang]

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

> Speaking of Error API -- what is the general consensus of this?  We have
> TONS of various way to report errors now, and we had several incarnations
> of various error APIs too, are we going to use some common API finally,
> or is the conversion endless, expecting new APIs to arrive?
>

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.

> And one more thing.  Patch 4 does this:
>
> @@ -1388,8 +1388,8 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>       ret->pty = g_strdup(pty_name);
>       ret->has_pty = true;
>
> -    fprintf(stderr, "char device redirected to %s (label %s)\n",
> -            pty_name, id);
> +    error_report("char device redirected to %s (label %s)",
> +                 pty_name, id);
>
>       s = g_malloc0(sizeof(PtyCharDriver));
>       chr->opaque = s;
>
> This is not really correct.  First it is not really an error, it is
> an informational message.  But also, there are many scripts out there
> who expects this very format of this message to find the entry to
> that char device.  Converting this message to error_report() changes
> its format, so scrips will be unable to find the device anymore.
>
> Take a look at history of this place, -- I remember there was a rather
> hot discussion when the last part, "(label %)", has been added (initial
> message was without this label).  Initial suggestion was to change
> wordin to 'char device $LABEL redirected to $DEVICE', but even if it
> is much more readable and correct, we agreed to add that "label" part
> to the end - not that it preserves original message, but at least it
> makes less scripts to fail...
>

I got it, thanks very much for your explanation, and your patience;)

> So at least this hunk should not be applied.  I think this place
> deserves a comment.
>
> I'm sorry for being so picky, but I think I give enough reasons
> explaining why, and these reasons are serious enough.

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

Thanks,
zhanghailiang

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

[Michael Tokarev]

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

    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.

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 .

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

Thank you!

/mjt

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

[Michael Tokarev]

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

    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.

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 .

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

Thank you!

/mjt

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

[zhanghailiang]

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

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

[zhanghailiang]

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

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

[Markus Armbruster]

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

> 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;)
>
>> Speaking of Error API -- what is the general consensus of this?  We have
>> TONS of various way to report errors now, and we had several incarnations
>> of various error APIs too, are we going to use some common API finally,
>> or is the conversion endless, expecting new APIs to arrive?

I hope we're done screwing up error APIs, and now we "only" have to
convert code from the APIs we don't want used to the ones we do want
used.  Quote http://wiki.qemu.org/CodeTransitions#Error_reporting:

* Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
  you have a specific reason.  Prefer error_setg() & friends over
  error_set() & friends.

* The QError macros QERR_ are a hack to help with the transition to the
  current error.h API (human-readable message rather than JSON argument,
  see commit df1e608).  Avoid them in new code, use simple message
  strings instead.

* qerror_report() is a transitional interface to help with converting
  existing HMP commands to QMP.  It should not be used elsewhere.  Use
  Error objects instead with error_propagate() and error_setg() instead.

* Use error_report() & friends instead of fprintf(stderr, ...).  Unlike
  fprintf(), it does the right thing in human monitor context.  It also
  adds program name and error location when appropriate, and supports
  timestamps (-msg timestamp=on).

End quote.  More questions?

> 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.
>
>> And one more thing.  Patch 4 does this:
>>
>> @@ -1388,8 +1388,8 @@ static CharDriverState
>> *qemu_chr_open_pty(const char *id,
>>       ret->pty = g_strdup(pty_name);
>>       ret->has_pty = true;
>>
>> -    fprintf(stderr, "char device redirected to %s (label %s)\n",
>> -            pty_name, id);
>> +    error_report("char device redirected to %s (label %s)",
>> +                 pty_name, id);
>>
>>       s = g_malloc0(sizeof(PtyCharDriver));
>>       chr->opaque = s;
>>
>> This is not really correct.  First it is not really an error, it is
>> an informational message.  But also, there are many scripts out there
>> who expects this very format of this message to find the entry to
>> that char device.  Converting this message to error_report() changes
>> its format, so scrips will be unable to find the device anymore.
>>
>> Take a look at history of this place, -- I remember there was a rather
>> hot discussion when the last part, "(label %)", has been added (initial
>> message was without this label).  Initial suggestion was to change
>> wordin to 'char device $LABEL redirected to $DEVICE', but even if it
>> is much more readable and correct, we agreed to add that "label" part
>> to the end - not that it preserves original message, but at least it
>> makes less scripts to fail...
>>
>
> I got it, thanks very much for your explanation, and your patience;)

Please use error_printf(), so it prints to the monitor when running on
behalf of monitor command chardev-add.  Separate patch, because it's a
separate bug fix.

>> So at least this hunk should not be applied.  I think this place
>> deserves a comment.
>>
>> I'm sorry for being so picky, but I think I give enough reasons
>> explaining why, and these reasons are serious enough.
>
> 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;)

Sounds like a plan to me.

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

[Markus Armbruster]

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

> 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;)
>
>> Speaking of Error API -- what is the general consensus of this?  We have
>> TONS of various way to report errors now, and we had several incarnations
>> of various error APIs too, are we going to use some common API finally,
>> or is the conversion endless, expecting new APIs to arrive?

I hope we're done screwing up error APIs, and now we "only" have to
convert code from the APIs we don't want used to the ones we do want
used.  Quote http://wiki.qemu.org/CodeTransitions#Error_reporting:

* Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
  you have a specific reason.  Prefer error_setg() & friends over
  error_set() & friends.

* The QError macros QERR_ are a hack to help with the transition to the
  current error.h API (human-readable message rather than JSON argument,
  see commit df1e608).  Avoid them in new code, use simple message
  strings instead.

* qerror_report() is a transitional interface to help with converting
  existing HMP commands to QMP.  It should not be used elsewhere.  Use
  Error objects instead with error_propagate() and error_setg() instead.

* Use error_report() & friends instead of fprintf(stderr, ...).  Unlike
  fprintf(), it does the right thing in human monitor context.  It also
  adds program name and error location when appropriate, and supports
  timestamps (-msg timestamp=on).

End quote.  More questions?

> 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.
>
>> And one more thing.  Patch 4 does this:
>>
>> @@ -1388,8 +1388,8 @@ static CharDriverState
>> *qemu_chr_open_pty(const char *id,
>>       ret->pty = g_strdup(pty_name);
>>       ret->has_pty = true;
>>
>> -    fprintf(stderr, "char device redirected to %s (label %s)\n",
>> -            pty_name, id);
>> +    error_report("char device redirected to %s (label %s)",
>> +                 pty_name, id);
>>
>>       s = g_malloc0(sizeof(PtyCharDriver));
>>       chr->opaque = s;
>>
>> This is not really correct.  First it is not really an error, it is
>> an informational message.  But also, there are many scripts out there
>> who expects this very format of this message to find the entry to
>> that char device.  Converting this message to error_report() changes
>> its format, so scrips will be unable to find the device anymore.
>>
>> Take a look at history of this place, -- I remember there was a rather
>> hot discussion when the last part, "(label %)", has been added (initial
>> message was without this label).  Initial suggestion was to change
>> wordin to 'char device $LABEL redirected to $DEVICE', but even if it
>> is much more readable and correct, we agreed to add that "label" part
>> to the end - not that it preserves original message, but at least it
>> makes less scripts to fail...
>>
>
> I got it, thanks very much for your explanation, and your patience;)

Please use error_printf(), so it prints to the monitor when running on
behalf of monitor command chardev-add.  Separate patch, because it's a
separate bug fix.

>> So at least this hunk should not be applied.  I think this place
>> deserves a comment.
>>
>> I'm sorry for being so picky, but I think I give enough reasons
>> explaining why, and these reasons are serious enough.
>
> 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;)

Sounds like a plan to me.

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

[zhanghailiang]

On 2014/11/3 23:33, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> 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;)
>>
>>> Speaking of Error API -- what is the general consensus of this?  We have
>>> TONS of various way to report errors now, and we had several incarnations
>>> of various error APIs too, are we going to use some common API finally,
>>> or is the conversion endless, expecting new APIs to arrive?
>
> I hope we're done screwing up error APIs, and now we "only" have to
> convert code from the APIs we don't want used to the ones we do want
> used.  Quote http://wiki.qemu.org/CodeTransitions#Error_reporting:
>
> * Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
>    you have a specific reason.  Prefer error_setg() & friends over
>    error_set() & friends.
>
> * The QError macros QERR_ are a hack to help with the transition to the
>    current error.h API (human-readable message rather than JSON argument,
>    see commit df1e608).  Avoid them in new code, use simple message
>    strings instead.
>
> * qerror_report() is a transitional interface to help with converting
>    existing HMP commands to QMP.  It should not be used elsewhere.  Use
>    Error objects instead with error_propagate() and error_setg() instead.
>
> * Use error_report() & friends instead of fprintf(stderr, ...).  Unlike
>    fprintf(), it does the right thing in human monitor context.  It also
>    adds program name and error location when appropriate, and supports
>    timestamps (-msg timestamp=on).
>
> End quote.  More questions?
>
>> 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.
>>
>>> And one more thing.  Patch 4 does this:
>>>
>>> @@ -1388,8 +1388,8 @@ static CharDriverState
>>> *qemu_chr_open_pty(const char *id,
>>>        ret->pty = g_strdup(pty_name);
>>>        ret->has_pty = true;
>>>
>>> -    fprintf(stderr, "char device redirected to %s (label %s)\n",
>>> -            pty_name, id);
>>> +    error_report("char device redirected to %s (label %s)",
>>> +                 pty_name, id);
>>>
>>>        s = g_malloc0(sizeof(PtyCharDriver));
>>>        chr->opaque = s;
>>>
>>> This is not really correct.  First it is not really an error, it is
>>> an informational message.  But also, there are many scripts out there
>>> who expects this very format of this message to find the entry to
>>> that char device.  Converting this message to error_report() changes
>>> its format, so scrips will be unable to find the device anymore.
>>>
>>> Take a look at history of this place, -- I remember there was a rather
>>> hot discussion when the last part, "(label %)", has been added (initial
>>> message was without this label).  Initial suggestion was to change
>>> wordin to 'char device $LABEL redirected to $DEVICE', but even if it
>>> is much more readable and correct, we agreed to add that "label" part
>>> to the end - not that it preserves original message, but at least it
>>> makes less scripts to fail...
>>>
>>
>> I got it, thanks very much for your explanation, and your patience;)
>
> Please use error_printf(), so it prints to the monitor when running on

Ah, i didn't notice this function before, great;) thanks.

> behalf of monitor command chardev-add.  Separate patch, because it's a
> separate bug fix.
>

OK, will do in V3.

>>> So at least this hunk should not be applied.  I think this place
>>> deserves a comment.
>>>
>>> I'm sorry for being so picky, but I think I give enough reasons
>>> explaining why, and these reasons are serious enough.
>>
>> 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;)
>
> Sounds like a plan to me.
> .
>

Thanks,
zhanghailiang

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

[zhanghailiang]

On 2014/11/3 23:33, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> 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;)
>>
>>> Speaking of Error API -- what is the general consensus of this?  We have
>>> TONS of various way to report errors now, and we had several incarnations
>>> of various error APIs too, are we going to use some common API finally,
>>> or is the conversion endless, expecting new APIs to arrive?
>
> I hope we're done screwing up error APIs, and now we "only" have to
> convert code from the APIs we don't want used to the ones we do want
> used.  Quote http://wiki.qemu.org/CodeTransitions#Error_reporting:
>
> * Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
>    you have a specific reason.  Prefer error_setg() & friends over
>    error_set() & friends.
>
> * The QError macros QERR_ are a hack to help with the transition to the
>    current error.h API (human-readable message rather than JSON argument,
>    see commit df1e608).  Avoid them in new code, use simple message
>    strings instead.
>
> * qerror_report() is a transitional interface to help with converting
>    existing HMP commands to QMP.  It should not be used elsewhere.  Use
>    Error objects instead with error_propagate() and error_setg() instead.
>
> * Use error_report() & friends instead of fprintf(stderr, ...).  Unlike
>    fprintf(), it does the right thing in human monitor context.  It also
>    adds program name and error location when appropriate, and supports
>    timestamps (-msg timestamp=on).
>
> End quote.  More questions?
>
>> 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.
>>
>>> And one more thing.  Patch 4 does this:
>>>
>>> @@ -1388,8 +1388,8 @@ static CharDriverState
>>> *qemu_chr_open_pty(const char *id,
>>>        ret->pty = g_strdup(pty_name);
>>>        ret->has_pty = true;
>>>
>>> -    fprintf(stderr, "char device redirected to %s (label %s)\n",
>>> -            pty_name, id);
>>> +    error_report("char device redirected to %s (label %s)",
>>> +                 pty_name, id);
>>>
>>>        s = g_malloc0(sizeof(PtyCharDriver));
>>>        chr->opaque = s;
>>>
>>> This is not really correct.  First it is not really an error, it is
>>> an informational message.  But also, there are many scripts out there
>>> who expects this very format of this message to find the entry to
>>> that char device.  Converting this message to error_report() changes
>>> its format, so scrips will be unable to find the device anymore.
>>>
>>> Take a look at history of this place, -- I remember there was a rather
>>> hot discussion when the last part, "(label %)", has been added (initial
>>> message was without this label).  Initial suggestion was to change
>>> wordin to 'char device $LABEL redirected to $DEVICE', but even if it
>>> is much more readable and correct, we agreed to add that "label" part
>>> to the end - not that it preserves original message, but at least it
>>> makes less scripts to fail...
>>>
>>
>> I got it, thanks very much for your explanation, and your patience;)
>
> Please use error_printf(), so it prints to the monitor when running on

Ah, i didn't notice this function before, great;) thanks.

> behalf of monitor command chardev-add.  Separate patch, because it's a
> separate bug fix.
>

OK, will do in V3.

>>> So at least this hunk should not be applied.  I think this place
>>> deserves a comment.
>>>
>>> I'm sorry for being so picky, but I think I give enough reasons
>>> explaining why, and these reasons are serious enough.
>>
>> 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;)
>
> Sounds like a plan to me.
> .
>

Thanks,
zhanghailiang