[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: 3/7:



On Tue, Aug 12, 2008 at 11:07:00AM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange redhat com> wrote:
> > diff -r 8093fb566748 src/lxc_conf.c
> > diff -r 8093fb566748 src/lxc_conf.h
> > --- a/src/lxc_conf.h	Fri Aug 01 14:47:33 2008 +0100
> > +++ b/src/lxc_conf.h	Tue Aug 05 12:13:24 2008 +0100
> > @@ -46,7 +46,6 @@
> >  struct __lxc_net_def {
> >      int type;
> >      char *parentVeth;       /* veth device in parent namespace */
> > -    char *containerVeth;    /* veth device in container namespace */
> >      char *txName;           /* bridge or network name */
> >
> >      lxc_net_def_t *next;
> > @@ -87,11 +86,10 @@
> >  struct __lxc_vm {
> >      int pid;
> >      int state;
> > +    int monitor;
> 
> I like "monitor_fd" ;-)

This struct goes away in the next patch.

> 
> > diff -r 8093fb566748 src/lxc_container.c
> > --- a/src/lxc_container.c	Fri Aug 01 14:47:33 2008 +0100
> > +++ b/src/lxc_container.c	Tue Aug 05 12:13:24 2008 +0100
> > @@ -69,6 +69,8 @@
> >  typedef struct __lxc_child_argv lxc_child_argv_t;
> >  struct __lxc_child_argv {
> >      lxc_vm_def_t *config;
> > +    int nveths;
> 
> From the looks of all uses, this can be an unsigned type.

Yes, I've updated this to be unsigned throughout.

> > @@ -230,21 +231,21 @@
> >   *
> >   * Returns 0 on success or nonzero in case of error
> >   */
> > -static int lxcContainerEnableInterfaces(const lxc_vm_def_t *def)
> > +static int lxcContainerEnableInterfaces(int nveths,
> > +                                        char **veths)
> >  {
> > -    int rc = 0;
> > -    const lxc_net_def_t *net;
> > +    int rc = 0, i;
> 
> Many style guides recommend against putting multiple ","-separated
> declarations on the same line...

I've split this onto 2 declarations anyway, because 'i'
can now be unsigned.

> > @@ -337,12 +338,11 @@
> >      int flags;
> >      int stacksize = getpagesize() * 4;
> >      char *stack, *stacktop;
> > -    lxc_child_argv_t args = { def, control, ttyPath };
> > +    lxc_child_argv_t args = { def, nveths, veths, control, ttyPath };
> 
> If possible, it'd be nice to make the struct "const".

Doesn't help much, because then I have to cast-away the
constness when passing it into clone().

> > @@ -379,8 +379,9 @@
> >      char *stack;
> >      int childStatus;
> >
> > -    if (features & LXC_CONTAINER_FEATURE_NET)
> > +    if (features & LXC_CONTAINER_FEATURE_NET) {
> >          flags |= CLONE_NEWNET;
> > +    }
> 
> Since you're making a change like this, I suspect it's worth adding a
> sentence+example to HACKING saying that we prefer to use braces even
> when there's only one statement.  Out of habit, I tend *not* to use
> braces in that case, but have no trouble adapting.  Codifying it might
> help avoid a little churn.

No, that's a bogus change - I don't like to have {} for single
line statements - its just unneccessary noise.

> > @@ -138,23 +198,46 @@
> >          /* if active fd's, return if no events, else wait forever */
> >          timeout = (numActive > 0) ? 0 : -1;
> >          numEvents = epoll_wait(epollFd, &epollEvent, 1, timeout);
> > -        if (0 < numEvents) {
> > -            if (epollEvent.events & EPOLLIN) {
> > -                curFdOff = epollEvent.data.u32;
> > -                if (!fdArray[curFdOff].active) {
> > -                    fdArray[curFdOff].active = 1;
> > -                    ++numActive;
> > +        if (numEvents > 0) {
> > +            if (epollEvent.data.fd == monitor) {
> > +                int fd = accept(monitor, NULL, 0);
> > +                if (client != -1 || /* Already connected, so kick new one out */
> 
> I presume you meant to insert "client = fd;" right after "fd = ...",
> (or compare fd != -1, and then set client = fd after the "}"),
> since it looks like "client" must be defined after this if/else chain.

Yes, there should have been  a   'client = fd;' statement after
the conditional.

> > +    kill(container, SIGTERM);
> > +    waitpid(container, NULL, 0);
> 
> It might be worthwhile to detect kill or waitpid failure when rc == 0.

Any more to the point, it should not be invoked at all when
container == -1, because that kills off every process on your
system except for init. Yes, that's happened to me several
times while debugging this :-)

> > + * container exits.
> > + *
> > + * Returns 0 on success or -1 in case of error
> > + */
> > +static int lxcVMCleanup(virConnectPtr conn,
> > +                        lxc_driver_t *driver,
> > +                        lxc_vm_t * vm)
> > +{
> > +    int rc = -1;
> > +    int waitRc;
> > +    int childStatus = -1;
> > +
> > +    while (((waitRc = waitpid(vm->pid, &childStatus, 0)) == -1) &&
> > +           errno == EINTR);
> 
> It's easier to recognize this as an empty loop if you give
> it a comment and put the semicolon on its own line:
> 
>        while (((waitRc = waitpid(vm->pid, &childStatus, 0)) == -1) &&
>               errno == EINTR)
>           ; /* empty */

Made this change.

> > +        goto error;
> > +    }
> > +
> > +    memset(&addr, 0, sizeof(addr));
> > +    addr.sun_family = AF_UNIX;
> > +    strncpy(addr.sun_path, sockpath, sizeof(addr.sun_path));
> > +
> > +    if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
> > +        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> > +                 _("failed to connect to client socket: %s"),
> > +                 strerror(errno));
> > +        goto error;
> > +    }
> ...
> 
> I'm quoting the following snippet here (may be out of order),
> because I spotted the problem only after eliding the original.
> 
> > +    /* And get its pid */
> > +    if ((rc = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) != 0> ) {
> >          lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> > -                 _("sockpair failed: %s"), strerror(errno));
> > +                 _("Failed to read pid file %s/%s.pid: %s"),
> > +                 driver->stateDir, vm->def->name, strerror(errno));
> 
> That should be "rc", not "errno".
> Otherwise, diagnosing the virFileReadPid parse error that is intended
> to map to EINVAL, we'd use some unrelated errno value.

Oh yes, changed that to 'rc'.

> >  int lxcRegister(void)
> > diff -r 8093fb566748 src/util.c
> > --- a/src/util.c	Fri Aug 01 14:47:33 2008 +0100
> > +++ b/src/util.c	Tue Aug 05 12:13:24 2008 +0100
> > @@ -37,6 +37,7 @@
> >  #include <sys/wait.h>
> >  #endif
> >  #include <string.h>
> > +#include <termios.h>
> >  #include "c-ctype.h"
> >
> >  #ifdef HAVE_PATHS_H
> > @@ -556,6 +557,163 @@
> >      return 0;
> >  }
> >
> > +
> > +int virFileOpenTty(int *ttymaster,
> > +                   char **ttyName,
> > +                   int rawmode)
> > +{
> > +    int rc = -1;
> 
> As you know,
> this function is a portability "challenge" ;-)  (that's an understatement)
> I suppose we'll need to guard this function with #ifdef WITH_LXC.

The util.c file is intended to be generic code so I like to avoid
making it conditional based on WITH_LXC. Instead I've changed it
to be #ifdef __linux__, and left an empty stub which just reports
an error for the non-Linux case. This follows the pattern we use
with virExec too.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]