From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41038) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btTje-0003nj-8P for qemu-devel@nongnu.org; Mon, 10 Oct 2016 02:06:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1btTjZ-00087N-UJ for qemu-devel@nongnu.org; Mon, 10 Oct 2016 02:06:49 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:49563) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btTjY-00085B-US for qemu-devel@nongnu.org; Mon, 10 Oct 2016 02:06:45 -0400 References: <1475208369-13756-1-git-send-email-zhang.zhanghailiang@huawei.com> <57FB0759.2080401@huawei.com> <9bab6585-3de0-6f6b-ae4e-1e4191d40e47@cn.fujitsu.com> From: Hailiang Zhang Message-ID: <57FB2FDA.7000800@huawei.com> Date: Mon, 10 Oct 2016 14:06:18 +0800 MIME-Version: 1.0 In-Reply-To: <9bab6585-3de0-6f6b-ae4e-1e4191d40e47@cn.fujitsu.com> 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 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 >>> >>> 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 >>> >>> 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; >>>> } >>>> >>> >> >> >> >> . >> >