Re: [Libvir] [PATCH] lxc: handle SIGCHLD from exiting container
Jim Meyering wrote:
Dave Leskovec [EMAIL PROTECTED] wrote:
...
+#ifndef _SIGNAL_H
+#include signal.h
+#endif
In practice it's fine to include signal.h unconditionally,
and even multiple times. Have you encountered a version of signal.h
that may not be included twice? If so, it probably deserves a comment
with the details.
No, I don't have any special condition here. This is probably some past
conditioning resurfacing briefly. If I remember correctly, it had more to do
with compile efficiency rather than avoiding compile failures from multiple
inclusions.
Then don't bother.
gcc performs a handy optimization whereby it doesn't even open
the header file the second (and subsequent) time it's included, as
long as it's entire contents is wrapped in the usual sort of guard:
#ifndef SYM
#define SYM
...
#endif
Thanks Jim. I've attached an updated patch with those two changes. While
making these changes, I noticed that I missed updating the storage drivers state
driver table. I've fixed that as well.
--
Best Regards,
Dave Leskovec
IBM Linux Technology Center
Open Virtualization
---
qemud/qemud.c | 26 ++
src/driver.h|4 +
src/internal.h |2
src/libvirt.c | 11
src/libvirt_sym.version |1
src/lxc_driver.c| 114 +++-
src/qemu_driver.c |1
src/remote_internal.c |1
src/storage_driver.c|1
9 files changed, 120 insertions(+), 41 deletions(-)
Index: b/qemud/qemud.c
===
--- a/qemud/qemud.c 2008-04-28 02:09:52.0 -0700
+++ b/qemud/qemud.c 2008-04-30 15:34:29.0 -0700
@@ -109,16 +109,16 @@
static sig_atomic_t sig_errors = 0;
static int sig_lasterrno = 0;
-static void sig_handler(int sig) {
-unsigned char sigc = sig;
+static void sig_handler(int sig, siginfo_t * siginfo,
+void* context ATTRIBUTE_UNUSED) {
int origerrno;
int r;
-if (sig == SIGCHLD) /* We explicitly waitpid the child later */
-return;
+/* set the sig num in the struct */
+siginfo-si_signo = sig;
origerrno = errno;
-r = safewrite(sigwrite, sigc, 1);
+r = safewrite(sigwrite, siginfo, sizeof(*siginfo));
if (r == -1) {
sig_errors++;
sig_lasterrno = errno;
@@ -232,10 +232,10 @@
int events ATTRIBUTE_UNUSED,
void *opaque) {
struct qemud_server *server = (struct qemud_server *)opaque;
-unsigned char sigc;
+siginfo_t siginfo;
int ret;
-if (read(server-sigread, sigc, 1) != 1) {
+if (read(server-sigread, siginfo, sizeof(siginfo)) != sizeof(siginfo)) {
qemudLog(QEMUD_ERR, _(Failed to read from signal pipe: %s),
strerror(errno));
return;
@@ -243,7 +243,7 @@
ret = 0;
-switch (sigc) {
+switch (siginfo.si_signo) {
case SIGHUP:
qemudLog(QEMUD_INFO, %s, _(Reloading configuration on SIGHUP));
if (virStateReload() 0)
@@ -253,11 +253,15 @@
case SIGINT:
case SIGQUIT:
case SIGTERM:
-qemudLog(QEMUD_WARN, _(Shutting down on signal %d), sigc);
+qemudLog(QEMUD_WARN, _(Shutting down on signal %d),
+ siginfo.si_signo);
server-shutdown = 1;
break;
default:
+qemudLog(QEMUD_INFO, _(Received signal %d, dispatching to drivers),
+ siginfo.si_signo);
+virStateSigDispatcher(siginfo);
break;
}
@@ -2186,8 +2190,8 @@
goto error1;
}
-sig_action.sa_handler = sig_handler;
-sig_action.sa_flags = 0;
+sig_action.sa_sigaction = sig_handler;
+sig_action.sa_flags = SA_SIGINFO;
sigemptyset(sig_action.sa_mask);
sigaction(SIGHUP, sig_action, NULL);
Index: b/src/driver.h
===
--- a/src/driver.h 2008-04-10 09:54:54.0 -0700
+++ b/src/driver.h 2008-05-05 23:28:40.0 -0700
@@ -11,6 +11,8 @@
#include libxml/uri.h
+#include signal.h
+
#ifdef __cplusplus
extern C {
#endif
@@ -565,6 +567,7 @@
typedef int (*virDrvStateCleanup) (void);
typedef int (*virDrvStateReload) (void);
typedef int (*virDrvStateActive) (void);
+typedef int (*virDrvSigHandler) (siginfo_t *siginfo);
typedef struct _virStateDriver virStateDriver;
typedef virStateDriver *virStateDriverPtr;
@@ -574,6 +577,7 @@
virDrvStateCleanup cleanup;
virDrvStateReload reload;
virDrvStateActive active;
+virDrvSigHandler sigHandler;
};
/*
Index: b/src/internal.h
===
--- a/src/internal.h 2008-04-28 14:44:54.0 -0700
+++ b/src/internal.h 2008-04-30 15:01:24.0 -0700
@@ -344,10 +344,12 @@
int __virStateCleanup(void);
int __virStateReload(void);
Re: [Libvir] [PATCH] lxc: handle SIGCHLD from exiting container
Dave Leskovec [EMAIL PROTECTED] wrote:
Jim Meyering wrote:
Dave Leskovec [EMAIL PROTECTED] wrote:
...
+#ifndef _SIGNAL_H
+#include signal.h
+#endif
In practice it's fine to include signal.h unconditionally,
and even multiple times. Have you encountered a version of signal.h
that may not be included twice? If so, it probably deserves a comment
with the details.
No, I don't have any special condition here. This is probably some past
conditioning resurfacing briefly. If I remember correctly, it had more to
do
with compile efficiency rather than avoiding compile failures from multiple
inclusions.
Then don't bother.
gcc performs a handy optimization whereby it doesn't even open
the header file the second (and subsequent) time it's included, as
long as it's entire contents is wrapped in the usual sort of guard:
#ifndef SYM
#define SYM
...
#endif
Thanks Jim. I've attached an updated patch with those two changes. While
making these changes, I noticed that I missed updating the storage drivers
state
driver table. I've fixed that as well.
--
...
Index: b/qemud/qemud.c
===
...
+static void sig_handler(int sig, siginfo_t * siginfo,
+void* context ATTRIBUTE_UNUSED) {
...
-unsigned char sigc;
+siginfo_t siginfo;
int ret;
-if (read(server-sigread, sigc, 1) != 1) {
+if (read(server-sigread, siginfo, sizeof(siginfo)) != sizeof(siginfo))
{
Looks good, but that should be saferead instead of read.
Now that it's reading more than one byte, EINTR can make a difference.
Also, it'd make it a tiny bit easier for people who reply to you
if you did not put code after your signature.
Or at least not after the -- signature-introducer.
Some mail clients (at least Gnus) -quote only the part
of your message that comes before the signature.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] lxc: handle SIGCHLD from exiting container
On Wed, Apr 30, 2008 at 11:38:01PM -0700, Dave Leskovec wrote: This patch allows the lxc driver to handle SIGCHLD signals from exiting containers. The handling will perform some cleanup such as waiting for the container process and killing/waiting the tty process. This is also required as a first step towards providing some kind of client container exit notification. Additional support is needed for that but this SIGCHLD handling is what would trigger the notification. libvirtd was already catching SIGCHLD although it was just ignoring it. I implemented a mechanism to distribute the signal to any other drivers in the daemon that registered a function to handle them. This required some changes to the way libvirtd was catching signals (to get the pid of the sending process) as well as an addition to the state driver structure. The intent was to provide future drivers access to signals as well. The reason it was ignoring it was because the QEMU driver detects the shutdown of the VM without using the SIGCHLD directly. It instead detects EOF on the STDOUT/ERR of the VM child process calls waitpid() then to cleanup. I notice that the LXC driver does not appear to setup any STDERR/OUT for its VMs so they're still inheriting the daemon's. If it isn't a huge problem it'd be desirable to try have QEMU LXC operate in the same general way wrt to their primary child procs for VMs. Regards, Daniel. -- |: Red Hat, Engineering, Boston -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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] lxc: handle SIGCHLD from exiting container
Daniel P. Berrange wrote: On Wed, Apr 30, 2008 at 11:38:01PM -0700, Dave Leskovec wrote: This patch allows the lxc driver to handle SIGCHLD signals from exiting containers. The handling will perform some cleanup such as waiting for the container process and killing/waiting the tty process. This is also required as a first step towards providing some kind of client container exit notification. Additional support is needed for that but this SIGCHLD handling is what would trigger the notification. libvirtd was already catching SIGCHLD although it was just ignoring it. I implemented a mechanism to distribute the signal to any other drivers in the daemon that registered a function to handle them. This required some changes to the way libvirtd was catching signals (to get the pid of the sending process) as well as an addition to the state driver structure. The intent was to provide future drivers access to signals as well. The reason it was ignoring it was because the QEMU driver detects the shutdown of the VM without using the SIGCHLD directly. It instead detects EOF on the STDOUT/ERR of the VM child process calls waitpid() then to cleanup. I notice that the LXC driver does not appear to setup any STDERR/OUT for its VMs so they're still inheriting the daemon's. If it isn't a huge problem it'd be desirable to try have QEMU LXC operate in the same general way wrt to their primary child procs for VMs. Regards, Daniel. stdout/err for the container is set to the tty. Containers can be used in a non-VM fashion as well. Think of a container running a daemon process or a container running a job as a part of a job scheduler/distribution system. Wouldn't it be valid in these cases for the container close stdout/err while continuing to run? -- Best Regards, Dave Leskovec IBM Linux Technology Center Open Virtualization -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] lxc: handle SIGCHLD from exiting container
On Mon, May 05, 2008 at 02:33:09PM -0700, Dave Leskovec wrote: Daniel P. Berrange wrote: On Wed, Apr 30, 2008 at 11:38:01PM -0700, Dave Leskovec wrote: This patch allows the lxc driver to handle SIGCHLD signals from exiting containers. The handling will perform some cleanup such as waiting for the container process and killing/waiting the tty process. This is also required as a first step towards providing some kind of client container exit notification. Additional support is needed for that but this SIGCHLD handling is what would trigger the notification. libvirtd was already catching SIGCHLD although it was just ignoring it. I implemented a mechanism to distribute the signal to any other drivers in the daemon that registered a function to handle them. This required some changes to the way libvirtd was catching signals (to get the pid of the sending process) as well as an addition to the state driver structure. The intent was to provide future drivers access to signals as well. The reason it was ignoring it was because the QEMU driver detects the shutdown of the VM without using the SIGCHLD directly. It instead detects EOF on the STDOUT/ERR of the VM child process calls waitpid() then to cleanup. I notice that the LXC driver does not appear to setup any STDERR/OUT for its VMs so they're still inheriting the daemon's. If it isn't a huge problem it'd be desirable to try have QEMU LXC operate in the same general way wrt to their primary child procs for VMs. Regards, Daniel. stdout/err for the container is set to the tty. Containers can be used in a non-VM fashion as well. Think of a container running a daemon process or a container running a job as a part of a job scheduler/distribution system. Wouldn't it be valid in these cases for the container close stdout/err while continuing to run? Hmm, yes, that could be a reasonable use case. I see the key difference here is the the immediate child of libvirt *is* the startup application in the container which can be anything. So yes, we can't rely on its use of stderr/out, as we do with QEMU where the immediate child has defined behaviour Dan. -- |: Red Hat, Engineering, Boston -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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] lxc: handle SIGCHLD from exiting container
Hi Jim,
Thanks for the review. Answers below -
Jim Meyering wrote:
Dave Leskovec [EMAIL PROTECTED] wrote:
This patch allows the lxc driver to handle SIGCHLD signals from exiting
...
Hi Dave,
At least superficially, this looks fine.
Two questions:
Index: b/src/driver.h
===
--- a/src/driver.h 2008-04-10 09:54:54.0 -0700
+++ b/src/driver.h 2008-04-30 15:36:47.0 -0700
@@ -11,6 +11,10 @@
#include libxml/uri.h
+#ifndef _SIGNAL_H
+#include signal.h
+#endif
In practice it's fine to include signal.h unconditionally,
and even multiple times. Have you encountered a version of signal.h
that may not be included twice? If so, it probably deserves a comment
with the details.
No, I don't have any special condition here. This is probably some past
conditioning resurfacing briefly. If I remember correctly, it had more to do
with compile efficiency rather than avoiding compile failures from multiple
inclusions.
...
Index: b/src/lxc_driver.c
===
...
-static int lxcDomainDestroy(virDomainPtr dom)
+static int lxcVMCleanup(lxc_driver_t *driver, lxc_vm_t * vm)
{
int rc = -1;
...
-rc = WEXITSTATUS(childStatus);
-DEBUG(container exited with rc: %d, rc);
+if (WIFEXITED(childStatus)) {
+rc = WEXITSTATUS(childStatus);
+DEBUG(container exited with rc: %d, rc);
+}
+
+rc = 0;
Didn't you mean to initialize rc=0 before that if block?
If not, please add a comment saying why the child failure
doesn't affect the function's return value.
Nice. Yes that rc = 0 definitely shouldn't be there.
--
Best Regards,
Dave Leskovec
IBM Linux Technology Center
Open Virtualization
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] [PATCH] lxc: handle SIGCHLD from exiting container
Dave Leskovec [EMAIL PROTECTED] wrote: ... +#ifndef_SIGNAL_H +#include signal.h +#endif In practice it's fine to include signal.h unconditionally, and even multiple times. Have you encountered a version of signal.h that may not be included twice? If so, it probably deserves a comment with the details. No, I don't have any special condition here. This is probably some past conditioning resurfacing briefly. If I remember correctly, it had more to do with compile efficiency rather than avoiding compile failures from multiple inclusions. Then don't bother. gcc performs a handy optimization whereby it doesn't even open the header file the second (and subsequent) time it's included, as long as it's entire contents is wrapped in the usual sort of guard: #ifndef SYM #define SYM ... #endif -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
