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