On Tue, Aug 12, 2008 at 11:07:00AM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list