Re: [Qemu-devel] [PATCH COLO-Frame v8 23/34] tap: Make launch_script() public

[zhanghailiang]

On 2015/7/29 17:24, Jason Wang wrote:
> On 07/29/2015 05:17 PM, zhanghailiang wrote:
> > On 2015/7/29 16:57, Jason Wang wrote:
> > > On 07/29/2015 04:45 PM, zhanghailiang wrote:
> > > > We also change the parameters of launch_script().
> > >
> > > A quick question (I don't go through the codes tough). What's the plan
> > > for management(libvirt)? I believe some setup (iptables, fd creation)
> > > should be offloaded to management (libvirt)?
> >
> > Er, yes, the better way for setup is in libvirt, we didn't look into it
> > deeply, but it was in our TODO list before, since our first step is to
> > merge colo's qemu part,
> > if we realize colo proxy in qemu, it seems to be more simple than this
> > inconvenient way.
>
>
> Please consider this as early as possible. The issue is probably not
> convenience but security. Running qemu as root is dangerous, that's why
> most of the setup was done through management.

Agreed, but if we totally convert proxy to userspace, we will not use this 
setup way (Using
iptables command), it will be no problem, is it?

Thanks.

> > > Thanks
> > >
> > > > Cc: Stefan Hajnoczi <address@hidden>
> > > > Cc: Jason Wang <address@hidden>
> > > > Signed-off-by: zhanghailiang <address@hidden>
> > > > Signed-off-by: Li Zhijian <address@hidden>
> > > > ---
> > > >    include/net/tap.h |  2 ++
> > > >    net/tap.c         | 31 ++++++++++++++++++-------------
> > > >    2 files changed, 20 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/include/net/tap.h b/include/net/tap.h
> > > > index 5da4edc..ac99b31 100644
> > > > --- a/include/net/tap.h
> > > > +++ b/include/net/tap.h
> > > > @@ -38,4 +38,6 @@ int tap_get_fd(NetClientState *nc);
> > > >    struct vhost_net;
> > > >    struct vhost_net *tap_get_vhost_net(NetClientState *nc);
> > > >
> > > > +void launch_script(char *const args[], int fd, Error **errp);
> > > > +
> > > >    #endif /* QEMU_NET_TAP_H */
> > > > diff --git a/net/tap.c b/net/tap.c
> > > > index c2135cd..a715636 100644
> > > > --- a/net/tap.c
> > > > +++ b/net/tap.c
> > > > @@ -60,9 +60,6 @@ typedef struct TAPState {
> > > >        unsigned host_vnet_hdr_len;
> > > >    } TAPState;
> > > >
> > > > -static void launch_script(const char *setup_script, const char
> > > > *ifname,
> > > > -                          int fd, Error **errp);
> > > > -
> > > >    static void tap_send(void *opaque);
> > > >    static void tap_writable(void *opaque);
> > > >
> > > > @@ -305,7 +302,14 @@ static void tap_cleanup(NetClientState *nc)
> > > >        qemu_purge_queued_packets(nc);
> > > >
> > > >        if (s->down_script[0]) {
> > > > -        launch_script(s->down_script, s->down_script_arg, s->fd,
> > > > &err);
> > > > +        char *args[3];
> > > > +        char **parg;
> > > > +
> > > > +        parg = args;
> > > > +        *parg++ = (char *)s->down_script;
> > > > +        *parg++ = (char *)s->down_script_arg;
> > > > +        *parg = NULL;
> > > > +        launch_script(args, s->fd, &err);
> > > >            if (err) {
> > > >                error_report_err(err);
> > > >            }
> > > > @@ -382,12 +386,10 @@ static TAPState
> > > > *net_tap_fd_init(NetClientState *peer,
> > > >        return s;
> > > >    }
> > > >
> > > > -static void launch_script(const char *setup_script, const char
> > > > *ifname,
> > > > -                          int fd, Error **errp)
> > > > +void launch_script(char *const args[], int fd, Error **errp)
> > > >    {
> > > >        int pid, status;
> > > > -    char *args[3];
> > > > -    char **parg;
> > > > +    const char *setup_script = args[0];
> > > >
> > > >        /* try to launch network script */
> > > >        pid = fork();
> > > > @@ -404,10 +406,6 @@ static void launch_script(const char
> > > > *setup_script, const char *ifname,
> > > >                    close(i);
> > > >                }
> > > >            }
> > > > -        parg = args;
> > > > -        *parg++ = (char *)setup_script;
> > > > -        *parg++ = (char *)ifname;
> > > > -        *parg = NULL;
> > > >            execv(setup_script, args);
> > > >            _exit(1);
> > > >        } else {
> > > > @@ -611,7 +609,14 @@ static int net_tap_init(const NetdevTapOptions
> > > > *tap, int *vnet_hdr,
> > > >        if (setup_script &&
> > > >            setup_script[0] != '\0' &&
> > > >            strcmp(setup_script, "no") != 0) {
> > > > -        launch_script(setup_script, ifname, fd, &err);
> > > > +        char *args[3];
> > > > +        char **parg;
> > > > +        parg = args;
> > > > +        *parg++ = (char *)setup_script;
> > > > +        *parg++ = (char *)ifname;
> > > > +        *parg = NULL;
> > > > +
> > > > +        launch_script(args, fd, &err);
> > > >            if (err) {
> > > >                error_propagate(errp, err);
> > > >                close(fd);
> > >
> > >
> > > .
>
>
> .