From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btR2O-0008Mr-H3 for qemu-devel@nongnu.org; Sun, 09 Oct 2016 23:14:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1btR2K-0002Ny-CS for qemu-devel@nongnu.org; Sun, 09 Oct 2016 23:14:00 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:62854) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btR2J-0002Hq-Mk for qemu-devel@nongnu.org; Sun, 09 Oct 2016 23:13:56 -0400 References: <1475208369-13756-1-git-send-email-zhang.zhanghailiang@huawei.com> From: Hailiang Zhang Message-ID: <57FB0759.2080401@huawei.com> Date: Mon, 10 Oct 2016 11:13:29 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhang Chen , lizhijian@cn.fujitsu.com, jasowang@redhat.com Cc: qemu-devel@nongnu.org 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 > > 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 > > 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; >> } >> >