5:38 a.m.
On Tue, Aug 12, 2008 at 11:07:00AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)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 :|