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

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

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;
>>>>         }
>>>>
>>>
>>
>>
>>
>> .
>>
>