* [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() @ 2016-09-30 4:06 zhanghailiang 2016-10-10 2:52 ` Zhang Chen 2016-10-20 2:12 ` Jason Wang 0 siblings, 2 replies; 10+ messages in thread From: zhanghailiang @ 2016-09-30 4:06 UTC (permalink / raw) To: lizhijian, zhangchen.fnst, jasowang; +Cc: qemu-devel, zhanghailiang find_and_check_chardev() uses 'opts' member of CharDriverState to check if the chardev is 'socket' chardev or not, which the opts will be NULL if We add the chardev by qmp 'chardev-add' command. All the related info can be found in 'filename' member of CharDriverState, For tcp socket device, it will be like 'disconnected:tcp:9.61.1.8:9004,server' or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply check it to identify if it is a tcp socket char device. Besides, fix this helper function to return -1 while some errors happen. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> --- net/colo-compare.c | 54 ++++++++---------------------------------------------- 1 file changed, 8 insertions(+), 46 deletions(-) diff --git a/net/colo-compare.c b/net/colo-compare.c index 22b1da1..6693258 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -92,10 +92,6 @@ typedef struct CompareClass { ObjectClass parent_class; } CompareClass; -typedef struct CompareChardevProps { - bool is_socket; -} CompareChardevProps; - enum { PRIMARY_IN = 0, SECONDARY_IN, @@ -564,56 +560,22 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs) } } -static int compare_chardev_opts(void *opaque, - const char *name, const char *value, - Error **errp) -{ - CompareChardevProps *props = opaque; - - if (strcmp(name, "backend") == 0 && - strcmp(value, "socket") == 0) { - props->is_socket = true; - return 0; - } else if (strcmp(name, "host") == 0 || - (strcmp(name, "port") == 0) || - (strcmp(name, "server") == 0) || - (strcmp(name, "wait") == 0) || - (strcmp(name, "path") == 0)) { - return 0; - } else { - error_setg(errp, - "COLO-compare does not support a chardev with option %s=%s", - name, value); - return -1; - } -} - -/* - * Return 0 is success. - * Return 1 is failed. - */ static int find_and_check_chardev(CharDriverState **chr, char *chr_name, Error **errp) { - CompareChardevProps props; - *chr = qemu_chr_find(chr_name); if (*chr == NULL) { error_setg(errp, "Device '%s' not found", chr_name); - return 1; + return -1; } - memset(&props, 0, sizeof(props)); - if (qemu_opt_foreach((*chr)->opts, compare_chardev_opts, &props, errp)) { - return 1; - } + if (!strstr((*chr)->filename, "tcp")) { + error_setg(errp, "chardev \"%s\" is not a tcp socket, filename '%s'", + chr_name, (*chr)->filename); + return -1; - if (!props.is_socket) { - error_setg(errp, "chardev \"%s\" is not a tcp socket", - chr_name); - return 1; } return 0; } @@ -660,15 +622,15 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) return; } - if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp)) { + if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp) < 0) { return; } - if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp)) { + if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp) < 0) { return; } - if (find_and_check_chardev(&s->chr_out, s->outdev, errp)) { + if (find_and_check_chardev(&s->chr_out, s->outdev, errp) < 0) { return; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-09-30 4:06 [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() zhanghailiang @ 2016-10-10 2:52 ` Zhang Chen 2016-10-10 3:13 ` Hailiang Zhang 2016-10-20 2:12 ` Jason Wang 1 sibling, 1 reply; 10+ messages in thread From: Zhang Chen @ 2016-10-10 2:52 UTC (permalink / raw) To: zhanghailiang, lizhijian, jasowang; +Cc: qemu-devel On 09/30/2016 12:06 PM, zhanghailiang wrote: > find_and_check_chardev() uses 'opts' member of CharDriverState to > check if the chardev is 'socket' chardev or not, which the opts > will be NULL if We add the chardev by qmp 'chardev-add' command. > > All the related info can be found in 'filename' member of CharDriverState, > For tcp socket device, it will be like 'disconnected:tcp:9.61.1.8:9004,server' > or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply check it to > identify if it is a tcp socket char device. > > Besides, fix this helper function to return -1 while some errors happen. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> This patch looks fine to me. Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> Thanks Zhang Chen > --- > net/colo-compare.c | 54 ++++++++---------------------------------------------- > 1 file changed, 8 insertions(+), 46 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index 22b1da1..6693258 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -92,10 +92,6 @@ typedef struct CompareClass { > ObjectClass parent_class; > } CompareClass; > > -typedef struct CompareChardevProps { > - bool is_socket; > -} CompareChardevProps; > - > enum { > PRIMARY_IN = 0, > SECONDARY_IN, > @@ -564,56 +560,22 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs) > } > } > > -static int compare_chardev_opts(void *opaque, > - const char *name, const char *value, > - Error **errp) > -{ > - CompareChardevProps *props = opaque; > - > - if (strcmp(name, "backend") == 0 && > - strcmp(value, "socket") == 0) { > - props->is_socket = true; > - return 0; > - } else if (strcmp(name, "host") == 0 || > - (strcmp(name, "port") == 0) || > - (strcmp(name, "server") == 0) || > - (strcmp(name, "wait") == 0) || > - (strcmp(name, "path") == 0)) { > - return 0; > - } else { > - error_setg(errp, > - "COLO-compare does not support a chardev with option %s=%s", > - name, value); > - return -1; > - } > -} > - > -/* > - * Return 0 is success. > - * Return 1 is failed. > - */ > static int find_and_check_chardev(CharDriverState **chr, > char *chr_name, > Error **errp) > { > - CompareChardevProps props; > - > *chr = qemu_chr_find(chr_name); > if (*chr == NULL) { > error_setg(errp, "Device '%s' not found", > chr_name); > - return 1; > + return -1; > } > > - memset(&props, 0, sizeof(props)); > - if (qemu_opt_foreach((*chr)->opts, compare_chardev_opts, &props, errp)) { > - return 1; > - } > + if (!strstr((*chr)->filename, "tcp")) { > + error_setg(errp, "chardev \"%s\" is not a tcp socket, filename '%s'", > + chr_name, (*chr)->filename); > + return -1; > > - if (!props.is_socket) { > - error_setg(errp, "chardev \"%s\" is not a tcp socket", > - chr_name); > - return 1; > } > return 0; > } > @@ -660,15 +622,15 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) > return; > } > > - if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp)) { > + if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp) < 0) { > return; > } > > - if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp)) { > + if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp) < 0) { > return; > } > > - if (find_and_check_chardev(&s->chr_out, s->outdev, errp)) { > + if (find_and_check_chardev(&s->chr_out, s->outdev, errp) < 0) { > return; > } > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-10-10 2:52 ` Zhang Chen @ 2016-10-10 3:13 ` Hailiang Zhang 2016-10-10 3:49 ` Zhang Chen 0 siblings, 1 reply; 10+ messages in thread From: Hailiang Zhang @ 2016-10-10 3:13 UTC (permalink / raw) To: Zhang Chen, lizhijian, jasowang; +Cc: qemu-devel Hi, On 2016/10/10 10:52, Zhang Chen wrote: > > > On 09/30/2016 12:06 PM, zhanghailiang wrote: >> find_and_check_chardev() uses 'opts' member of CharDriverState to >> check if the chardev is 'socket' chardev or not, which the opts >> will be NULL if We add the chardev by qmp 'chardev-add' command. >> >> All the related info can be found in 'filename' member of CharDriverState, >> For tcp socket device, it will be like 'disconnected:tcp:9.61.1.8:9004,server' >> or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply check it to >> identify if it is a tcp socket char device. >> >> Besides, fix this helper function to return -1 while some errors happen. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > > This patch looks fine to me. > Sorry, I found there are still some problems with this modification, For some local connection between filter objects, I think we can use unix socket instead of tcp socket. (Or even other char device, for example file or pipe, but Let's make things simple, we limit it to socket now) So the below check is insufficient, It should be + if (!strstr((*chr)->filename, "tcp:") && !strstr((*chr)->filename, "unix:")) { error_setg(errp, "chardev \"%s\" is not a tcp socket, filename '%s'", chr_name, (*chr)->filename); If you and Jason agree with this, i will send V2. Thanks, Hailiang > Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > > Thanks > Zhang Chen > >> --- >> net/colo-compare.c | 54 ++++++++---------------------------------------------- >> 1 file changed, 8 insertions(+), 46 deletions(-) >> >> diff --git a/net/colo-compare.c b/net/colo-compare.c >> index 22b1da1..6693258 100644 >> --- a/net/colo-compare.c >> +++ b/net/colo-compare.c >> @@ -92,10 +92,6 @@ typedef struct CompareClass { >> ObjectClass parent_class; >> } CompareClass; >> >> -typedef struct CompareChardevProps { >> - bool is_socket; >> -} CompareChardevProps; >> - >> enum { >> PRIMARY_IN = 0, >> SECONDARY_IN, >> @@ -564,56 +560,22 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs) >> } >> } >> >> -static int compare_chardev_opts(void *opaque, >> - const char *name, const char *value, >> - Error **errp) >> -{ >> - CompareChardevProps *props = opaque; >> - >> - if (strcmp(name, "backend") == 0 && >> - strcmp(value, "socket") == 0) { >> - props->is_socket = true; >> - return 0; >> - } else if (strcmp(name, "host") == 0 || >> - (strcmp(name, "port") == 0) || >> - (strcmp(name, "server") == 0) || >> - (strcmp(name, "wait") == 0) || >> - (strcmp(name, "path") == 0)) { >> - return 0; >> - } else { >> - error_setg(errp, >> - "COLO-compare does not support a chardev with option %s=%s", >> - name, value); >> - return -1; >> - } >> -} >> - >> -/* >> - * Return 0 is success. >> - * Return 1 is failed. >> - */ >> static int find_and_check_chardev(CharDriverState **chr, >> char *chr_name, >> Error **errp) >> { >> - CompareChardevProps props; >> - >> *chr = qemu_chr_find(chr_name); >> if (*chr == NULL) { >> error_setg(errp, "Device '%s' not found", >> chr_name); >> - return 1; >> + return -1; >> } >> >> - memset(&props, 0, sizeof(props)); >> - if (qemu_opt_foreach((*chr)->opts, compare_chardev_opts, &props, errp)) { >> - return 1; >> - } >> + if (!strstr((*chr)->filename, "tcp")) { >> + error_setg(errp, "chardev \"%s\" is not a tcp socket, filename '%s'", >> + chr_name, (*chr)->filename); >> + return -1; >> >> - if (!props.is_socket) { >> - error_setg(errp, "chardev \"%s\" is not a tcp socket", >> - chr_name); >> - return 1; >> } >> return 0; >> } >> @@ -660,15 +622,15 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) >> return; >> } >> >> - if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp)) { >> + if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp) < 0) { >> return; >> } >> >> - if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp)) { >> + if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp) < 0) { >> return; >> } >> >> - if (find_and_check_chardev(&s->chr_out, s->outdev, errp)) { >> + if (find_and_check_chardev(&s->chr_out, s->outdev, errp) < 0) { >> return; >> } >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-10-10 3:13 ` Hailiang Zhang @ 2016-10-10 3:49 ` Zhang Chen 2016-10-10 6:06 ` Hailiang Zhang 2016-10-20 3:53 ` Jason Wang 0 siblings, 2 replies; 10+ messages in thread From: Zhang Chen @ 2016-10-10 3:49 UTC (permalink / raw) To: Hailiang Zhang, lizhijian, jasowang; +Cc: qemu-devel On 10/10/2016 11:13 AM, Hailiang Zhang wrote: > Hi, > > On 2016/10/10 10:52, Zhang Chen wrote: >> >> >> On 09/30/2016 12:06 PM, zhanghailiang wrote: >>> find_and_check_chardev() uses 'opts' member of CharDriverState to >>> check if the chardev is 'socket' chardev or not, which the opts >>> will be NULL if We add the chardev by qmp 'chardev-add' command. >>> >>> All the related info can be found in 'filename' member of >>> CharDriverState, >>> For tcp socket device, it will be like >>> 'disconnected:tcp:9.61.1.8:9004,server' >>> or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply >>> check it to >>> identify if it is a tcp socket char device. >>> >>> Besides, fix this helper function to return -1 while some errors >>> happen. >>> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> >> This patch looks fine to me. >> > > Sorry, I found there are still some problems with this modification, > For some local connection between filter objects, I think we can use > unix socket > instead of tcp socket. (Or even other char device, for example file or > pipe, but > Let's make things simple, we limit it to socket now) > > So the below check is insufficient, It should be > > + if (!strstr((*chr)->filename, "tcp:") && > !strstr((*chr)->filename, "unix:")) { > error_setg(errp, "chardev \"%s\" is not a tcp socket, > filename '%s'", > chr_name, (*chr)->filename); > > If you and Jason agree with this, i will send V2. > I find part of codes in this patch has same with another patch: net: don't poke at chardev internal QemuOpts I think you can fix and rebase your patch, then we need jason's comments for this. Thanks Zhang Chen > Thanks, > Hailiang > >> Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> >> Thanks >> Zhang Chen >> >>> --- >>> net/colo-compare.c | 54 >>> ++++++++---------------------------------------------- >>> 1 file changed, 8 insertions(+), 46 deletions(-) >>> >>> diff --git a/net/colo-compare.c b/net/colo-compare.c >>> index 22b1da1..6693258 100644 >>> --- a/net/colo-compare.c >>> +++ b/net/colo-compare.c >>> @@ -92,10 +92,6 @@ typedef struct CompareClass { >>> ObjectClass parent_class; >>> } CompareClass; >>> >>> -typedef struct CompareChardevProps { >>> - bool is_socket; >>> -} CompareChardevProps; >>> - >>> enum { >>> PRIMARY_IN = 0, >>> SECONDARY_IN, >>> @@ -564,56 +560,22 @@ static void >>> compare_sec_rs_finalize(SocketReadState *sec_rs) >>> } >>> } >>> >>> -static int compare_chardev_opts(void *opaque, >>> - const char *name, const char *value, >>> - Error **errp) >>> -{ >>> - CompareChardevProps *props = opaque; >>> - >>> - if (strcmp(name, "backend") == 0 && >>> - strcmp(value, "socket") == 0) { >>> - props->is_socket = true; >>> - return 0; >>> - } else if (strcmp(name, "host") == 0 || >>> - (strcmp(name, "port") == 0) || >>> - (strcmp(name, "server") == 0) || >>> - (strcmp(name, "wait") == 0) || >>> - (strcmp(name, "path") == 0)) { >>> - return 0; >>> - } else { >>> - error_setg(errp, >>> - "COLO-compare does not support a chardev with >>> option %s=%s", >>> - name, value); >>> - return -1; >>> - } >>> -} >>> - >>> -/* >>> - * Return 0 is success. >>> - * Return 1 is failed. >>> - */ >>> static int find_and_check_chardev(CharDriverState **chr, >>> char *chr_name, >>> Error **errp) >>> { >>> - CompareChardevProps props; >>> - >>> *chr = qemu_chr_find(chr_name); >>> if (*chr == NULL) { >>> error_setg(errp, "Device '%s' not found", >>> chr_name); >>> - return 1; >>> + return -1; >>> } >>> >>> - memset(&props, 0, sizeof(props)); >>> - if (qemu_opt_foreach((*chr)->opts, compare_chardev_opts, >>> &props, errp)) { >>> - return 1; >>> - } >>> + if (!strstr((*chr)->filename, "tcp")) { >>> + error_setg(errp, "chardev \"%s\" is not a tcp socket, >>> filename '%s'", >>> + chr_name, (*chr)->filename); >>> + return -1; >>> >>> - if (!props.is_socket) { >>> - error_setg(errp, "chardev \"%s\" is not a tcp socket", >>> - chr_name); >>> - return 1; >>> } >>> return 0; >>> } >>> @@ -660,15 +622,15 @@ static void >>> colo_compare_complete(UserCreatable *uc, Error **errp) >>> return; >>> } >>> >>> - if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp)) { >>> + if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp) >>> < 0) { >>> return; >>> } >>> >>> - if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp)) { >>> + if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp) >>> < 0) { >>> return; >>> } >>> >>> - if (find_and_check_chardev(&s->chr_out, s->outdev, errp)) { >>> + if (find_and_check_chardev(&s->chr_out, s->outdev, errp) < 0) { >>> return; >>> } >>> >> > > > > . > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-10-10 3:49 ` Zhang Chen @ 2016-10-10 6:06 ` Hailiang Zhang 2016-10-20 3:53 ` Jason Wang 1 sibling, 0 replies; 10+ messages in thread From: Hailiang Zhang @ 2016-10-10 6:06 UTC (permalink / raw) To: Zhang Chen, lizhijian, jasowang; +Cc: qemu-devel On 2016/10/10 11:49, Zhang Chen wrote: > > > On 10/10/2016 11:13 AM, Hailiang Zhang wrote: >> Hi, >> >> On 2016/10/10 10:52, Zhang Chen wrote: >>> >>> >>> On 09/30/2016 12:06 PM, zhanghailiang wrote: >>>> find_and_check_chardev() uses 'opts' member of CharDriverState to >>>> check if the chardev is 'socket' chardev or not, which the opts >>>> will be NULL if We add the chardev by qmp 'chardev-add' command. >>>> >>>> All the related info can be found in 'filename' member of >>>> CharDriverState, >>>> For tcp socket device, it will be like >>>> 'disconnected:tcp:9.61.1.8:9004,server' >>>> or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply >>>> check it to >>>> identify if it is a tcp socket char device. >>>> >>>> Besides, fix this helper function to return -1 while some errors >>>> happen. >>>> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> >>> This patch looks fine to me. >>> >> >> Sorry, I found there are still some problems with this modification, >> For some local connection between filter objects, I think we can use >> unix socket >> instead of tcp socket. (Or even other char device, for example file or >> pipe, but >> Let's make things simple, we limit it to socket now) >> >> So the below check is insufficient, It should be >> >> + if (!strstr((*chr)->filename, "tcp:") && >> !strstr((*chr)->filename, "unix:")) { >> error_setg(errp, "chardev \"%s\" is not a tcp socket, >> filename '%s'", >> chr_name, (*chr)->filename); >> >> If you and Jason agree with this, i will send V2. >> > > I find part of codes in this patch has same with another patch: > > net: don't poke at chardev internal QemuOpts > I have tested this patch, it can achieve the same function, So please ignore this patch, thanks. > I think you can fix and rebase your patch, > then we need jason's comments for this. > > > Thanks > Zhang Chen > >> Thanks, >> Hailiang >> >>> Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>> >>> Thanks >>> Zhang Chen >>> >>>> --- >>>> net/colo-compare.c | 54 >>>> ++++++++---------------------------------------------- >>>> 1 file changed, 8 insertions(+), 46 deletions(-) >>>> >>>> diff --git a/net/colo-compare.c b/net/colo-compare.c >>>> index 22b1da1..6693258 100644 >>>> --- a/net/colo-compare.c >>>> +++ b/net/colo-compare.c >>>> @@ -92,10 +92,6 @@ typedef struct CompareClass { >>>> ObjectClass parent_class; >>>> } CompareClass; >>>> >>>> -typedef struct CompareChardevProps { >>>> - bool is_socket; >>>> -} CompareChardevProps; >>>> - >>>> enum { >>>> PRIMARY_IN = 0, >>>> SECONDARY_IN, >>>> @@ -564,56 +560,22 @@ static void >>>> compare_sec_rs_finalize(SocketReadState *sec_rs) >>>> } >>>> } >>>> >>>> -static int compare_chardev_opts(void *opaque, >>>> - const char *name, const char *value, >>>> - Error **errp) >>>> -{ >>>> - CompareChardevProps *props = opaque; >>>> - >>>> - if (strcmp(name, "backend") == 0 && >>>> - strcmp(value, "socket") == 0) { >>>> - props->is_socket = true; >>>> - return 0; >>>> - } else if (strcmp(name, "host") == 0 || >>>> - (strcmp(name, "port") == 0) || >>>> - (strcmp(name, "server") == 0) || >>>> - (strcmp(name, "wait") == 0) || >>>> - (strcmp(name, "path") == 0)) { >>>> - return 0; >>>> - } else { >>>> - error_setg(errp, >>>> - "COLO-compare does not support a chardev with >>>> option %s=%s", >>>> - name, value); >>>> - return -1; >>>> - } >>>> -} >>>> - >>>> -/* >>>> - * Return 0 is success. >>>> - * Return 1 is failed. >>>> - */ >>>> static int find_and_check_chardev(CharDriverState **chr, >>>> char *chr_name, >>>> Error **errp) >>>> { >>>> - CompareChardevProps props; >>>> - >>>> *chr = qemu_chr_find(chr_name); >>>> if (*chr == NULL) { >>>> error_setg(errp, "Device '%s' not found", >>>> chr_name); >>>> - return 1; >>>> + return -1; >>>> } >>>> >>>> - memset(&props, 0, sizeof(props)); >>>> - if (qemu_opt_foreach((*chr)->opts, compare_chardev_opts, >>>> &props, errp)) { >>>> - return 1; >>>> - } >>>> + if (!strstr((*chr)->filename, "tcp")) { >>>> + error_setg(errp, "chardev \"%s\" is not a tcp socket, >>>> filename '%s'", >>>> + chr_name, (*chr)->filename); >>>> + return -1; >>>> >>>> - if (!props.is_socket) { >>>> - error_setg(errp, "chardev \"%s\" is not a tcp socket", >>>> - chr_name); >>>> - return 1; >>>> } >>>> return 0; >>>> } >>>> @@ -660,15 +622,15 @@ static void >>>> colo_compare_complete(UserCreatable *uc, Error **errp) >>>> return; >>>> } >>>> >>>> - if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp)) { >>>> + if (find_and_check_chardev(&s->chr_pri_in, s->pri_indev, errp) >>>> < 0) { >>>> return; >>>> } >>>> >>>> - if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp)) { >>>> + if (find_and_check_chardev(&s->chr_sec_in, s->sec_indev, errp) >>>> < 0) { >>>> return; >>>> } >>>> >>>> - if (find_and_check_chardev(&s->chr_out, s->outdev, errp)) { >>>> + if (find_and_check_chardev(&s->chr_out, s->outdev, errp) < 0) { >>>> return; >>>> } >>>> >>> >> >> >> >> . >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-10-10 3:49 ` Zhang Chen 2016-10-10 6:06 ` Hailiang Zhang @ 2016-10-20 3:53 ` Jason Wang 2016-10-20 4:53 ` Hailiang Zhang 1 sibling, 1 reply; 10+ messages in thread From: Jason Wang @ 2016-10-20 3:53 UTC (permalink / raw) To: Zhang Chen, Hailiang Zhang, lizhijian; +Cc: qemu-devel On 2016年10月10日 11:49, Zhang Chen wrote: > > > On 10/10/2016 11:13 AM, Hailiang Zhang wrote: >> Hi, >> >> On 2016/10/10 10:52, Zhang Chen wrote: >>> >>> >>> On 09/30/2016 12:06 PM, zhanghailiang wrote: >>>> find_and_check_chardev() uses 'opts' member of CharDriverState to >>>> check if the chardev is 'socket' chardev or not, which the opts >>>> will be NULL if We add the chardev by qmp 'chardev-add' command. >>>> >>>> All the related info can be found in 'filename' member of >>>> CharDriverState, >>>> For tcp socket device, it will be like >>>> 'disconnected:tcp:9.61.1.8:9004,server' >>>> or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply >>>> check it to >>>> identify if it is a tcp socket char device. >>>> >>>> Besides, fix this helper function to return -1 while some errors >>>> happen. >>>> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> >>> This patch looks fine to me. >>> >> >> Sorry, I found there are still some problems with this modification, >> For some local connection between filter objects, I think we can use >> unix socket >> instead of tcp socket. (Or even other char device, for example file >> or pipe, but >> Let's make things simple, we limit it to socket now) >> >> So the below check is insufficient, It should be >> >> + if (!strstr((*chr)->filename, "tcp:") && >> !strstr((*chr)->filename, "unix:")) { >> error_setg(errp, "chardev \"%s\" is not a tcp socket, >> filename '%s'", >> chr_name, (*chr)->filename); >> >> If you and Jason agree with this, i will send V2. >> > > I find part of codes in this patch has same with another patch: > > net: don't poke at chardev internal QemuOpts > > I think you can fix and rebase your patch, > then we need jason's comments for this. I think we don't support unix domain socket? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-10-20 3:53 ` Jason Wang @ 2016-10-20 4:53 ` Hailiang Zhang 2016-10-21 2:13 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Hailiang Zhang @ 2016-10-20 4:53 UTC (permalink / raw) To: Jason Wang, Zhang Chen, lizhijian; +Cc: qemu-devel On 2016/10/20 11:53, Jason Wang wrote: > > > On 2016年10月10日 11:49, Zhang Chen wrote: >> >> >> On 10/10/2016 11:13 AM, Hailiang Zhang wrote: >>> Hi, >>> >>> On 2016/10/10 10:52, Zhang Chen wrote: >>>> >>>> >>>> On 09/30/2016 12:06 PM, zhanghailiang wrote: >>>>> find_and_check_chardev() uses 'opts' member of CharDriverState to >>>>> check if the chardev is 'socket' chardev or not, which the opts >>>>> will be NULL if We add the chardev by qmp 'chardev-add' command. >>>>> >>>>> All the related info can be found in 'filename' member of >>>>> CharDriverState, >>>>> For tcp socket device, it will be like >>>>> 'disconnected:tcp:9.61.1.8:9004,server' >>>>> or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply >>>>> check it to >>>>> identify if it is a tcp socket char device. >>>>> >>>>> Besides, fix this helper function to return -1 while some errors >>>>> happen. >>>>> >>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> >>>> This patch looks fine to me. >>>> >>> >>> Sorry, I found there are still some problems with this modification, >>> For some local connection between filter objects, I think we can use >>> unix socket >>> instead of tcp socket. (Or even other char device, for example file >>> or pipe, but >>> Let's make things simple, we limit it to socket now) >>> >>> So the below check is insufficient, It should be >>> >>> + if (!strstr((*chr)->filename, "tcp:") && >>> !strstr((*chr)->filename, "unix:")) { >>> error_setg(errp, "chardev \"%s\" is not a tcp socket, >>> filename '%s'", >>> chr_name, (*chr)->filename); >>> >>> If you and Jason agree with this, i will send V2. >>> >> >> I find part of codes in this patch has same with another patch: >> >> net: don't poke at chardev internal QemuOpts >> >> I think you can fix and rebase your patch, >> then we need jason's comments for this. > > I think we don't support unix domain socket? > No, we support it, actually, by using unix socket, we can reduce the number of ports used in COLO process. > . > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-10-20 4:53 ` Hailiang Zhang @ 2016-10-21 2:13 ` Jason Wang 0 siblings, 0 replies; 10+ messages in thread From: Jason Wang @ 2016-10-21 2:13 UTC (permalink / raw) To: Hailiang Zhang, Zhang Chen, lizhijian; +Cc: qemu-devel On 2016年10月20日 12:53, Hailiang Zhang wrote: > On 2016/10/20 11:53, Jason Wang wrote: >> >> >> On 2016年10月10日 11:49, Zhang Chen wrote: >>> >>> >>> On 10/10/2016 11:13 AM, Hailiang Zhang wrote: >>>> Hi, >>>> >>>> On 2016/10/10 10:52, Zhang Chen wrote: >>>>> >>>>> >>>>> On 09/30/2016 12:06 PM, zhanghailiang wrote: >>>>>> find_and_check_chardev() uses 'opts' member of CharDriverState to >>>>>> check if the chardev is 'socket' chardev or not, which the opts >>>>>> will be NULL if We add the chardev by qmp 'chardev-add' command. >>>>>> >>>>>> All the related info can be found in 'filename' member of >>>>>> CharDriverState, >>>>>> For tcp socket device, it will be like >>>>>> 'disconnected:tcp:9.61.1.8:9004,server' >>>>>> or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply >>>>>> check it to >>>>>> identify if it is a tcp socket char device. >>>>>> >>>>>> Besides, fix this helper function to return -1 while some errors >>>>>> happen. >>>>>> >>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>>> >>>>> This patch looks fine to me. >>>>> >>>> >>>> Sorry, I found there are still some problems with this modification, >>>> For some local connection between filter objects, I think we can use >>>> unix socket >>>> instead of tcp socket. (Or even other char device, for example file >>>> or pipe, but >>>> Let's make things simple, we limit it to socket now) >>>> >>>> So the below check is insufficient, It should be >>>> >>>> + if (!strstr((*chr)->filename, "tcp:") && >>>> !strstr((*chr)->filename, "unix:")) { >>>> error_setg(errp, "chardev \"%s\" is not a tcp socket, >>>> filename '%s'", >>>> chr_name, (*chr)->filename); >>>> >>>> If you and Jason agree with this, i will send V2. >>>> >>> >>> I find part of codes in this patch has same with another patch: >>> >>> net: don't poke at chardev internal QemuOpts >>> >>> I think you can fix and rebase your patch, >>> then we need jason's comments for this. >> >> I think we don't support unix domain socket? >> > > No, we support it, actually, by using unix socket, > we can reduce the number of ports used in COLO process. Ok, I see. Thanks ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-09-30 4:06 [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() zhanghailiang 2016-10-10 2:52 ` Zhang Chen @ 2016-10-20 2:12 ` Jason Wang 2016-10-20 4:52 ` Hailiang Zhang 1 sibling, 1 reply; 10+ messages in thread From: Jason Wang @ 2016-10-20 2:12 UTC (permalink / raw) To: zhanghailiang, lizhijian, zhangchen.fnst; +Cc: qemu-devel On 2016年09月30日 12:06, zhanghailiang wrote: > find_and_check_chardev() uses 'opts' member of CharDriverState to > check if the chardev is 'socket' chardev or not, which the opts > will be NULL if We add the chardev by qmp 'chardev-add' command. > > All the related info can be found in 'filename' member of CharDriverState, > For tcp socket device, it will be like 'disconnected:tcp:9.61.1.8:9004,server' > or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply check it to > identify if it is a tcp socket char device. > > Besides, fix this helper function to return -1 while some errors happen. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> Do we have similar issue in net_vhost_chardev_opts() ? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() 2016-10-20 2:12 ` Jason Wang @ 2016-10-20 4:52 ` Hailiang Zhang 0 siblings, 0 replies; 10+ messages in thread From: Hailiang Zhang @ 2016-10-20 4:52 UTC (permalink / raw) To: Jason Wang, lizhijian, zhangchen.fnst; +Cc: qemu-devel Hi Jason, On 2016/10/20 10:12, Jason Wang wrote: > > > On 2016年09月30日 12:06, zhanghailiang wrote: >> find_and_check_chardev() uses 'opts' member of CharDriverState to >> check if the chardev is 'socket' chardev or not, which the opts >> will be NULL if We add the chardev by qmp 'chardev-add' command. >> >> All the related info can be found in 'filename' member of CharDriverState, >> For tcp socket device, it will be like 'disconnected:tcp:9.61.1.8:9004,server' >> or 'tcp:9.61.1.8:9001,server <-> 9.61.1.8:50256', we can simply check it to >> identify if it is a tcp socket char device. >> >> Besides, fix this helper function to return -1 while some errors happen. >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > > Do we have similar issue in net_vhost_chardev_opts() ? > Yes, you are right, both of them have been fixed by another patch 'net: don't poke at chardev internal QemuOpts' I have replied in another email. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-10-21 2:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-30 4:06 [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() zhanghailiang 2016-10-10 2:52 ` Zhang Chen 2016-10-10 3:13 ` Hailiang Zhang 2016-10-10 3:49 ` Zhang Chen 2016-10-10 6:06 ` Hailiang Zhang 2016-10-20 3:53 ` Jason Wang 2016-10-20 4:53 ` Hailiang Zhang 2016-10-21 2:13 ` Jason Wang 2016-10-20 2:12 ` Jason Wang 2016-10-20 4:52 ` Hailiang Zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.