[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: zhanghailiang
Subject: Re: [Qemu-trivial] [PATCH v2 0/5] Trivial patch about qemu-char
Date: Tue, 4 Nov 2014 10:17:07 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Thunderbird/31.1.1

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.

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


     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

     So check the parameters for emptiness too, and avoid emptiness
     check at open time.

     Signed-off-by: zhanghailiang <address@hidden>
     Signed-off-by: Michael Tokarev <address@hidden>

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

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

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]