Re: [Qemu-devel] [PATCH] colo-compare: fix find_and_check_chardev()

[Zhang Chen <zhangchen.fnst@cn.fujitsu.com>]

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