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

[Hailiang Zhang]

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 <address@hidden>
> > >
> > > 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 <address@hidden>
> > >
> > > 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;
> > > >         }
> >
> >
> >
> > .