[libvirt] [PATCH 1/3] Add APIs for talking to init via /dev/initctl

[Daniel P. Berrange]

On Thu, Nov 29, 2012 at 01:16:09PM -0500, Eric Blake wrote:
> > To be able todo controlled shutdown/reboot of containers an
> 
> s/todo/to do/
> 
> > API to talk to init via /dev/initctl is required. Fortunately
> > this is quite straightforward to implement, and is supported
> > by both sysvinit and systemd. Upstart support for /dev/initctl
> > is unclear.
> > 
> 
> > +++ b/src/util/virinitctl.c
> > @@ -0,0 +1,161 @@
> 
> > +
> > +/* These constants & struct definitions are taken from
> > + * systemd, under terms of LGPLv2+
> > + *
> > + * initreq.h    Interface to talk to init through /dev/initctl.
> > + *
> > + *              Copyright (C) 1995-2004 Miquel van Smoorenburg
> > + */
> 
> Thankfully, compatible with our usage.
> 
> > +
> > +#if defined(__FreeBSD_kernel__)
> > +# define VIR_INITCTL_FIFO  "/etc/.initctl"
> > +#else
> > +# define VIR_INITCTL_FIFO  "/dev/initctl"
> > +#endif
> > +
> > +#define VIR_INITCTL_MAGIC 0x03091969
> 
> Wonder if the original author of this code picked a birthdate.  Gotta
> love the Easter eggs present in open source software :)
> 
> > +
> > +/*
> > + *      Because of legacy interfaces, "runlevel" and "sleeptime"
> > + *      aren't in a separate struct in the union.
> > + *
> > + *      The weird sizes are because init expects the whole
> > + *      struct to be 384 bytes.
> > + */
> > +struct virInitctlRequest {
> > +    int     magic;                  /* Magic number
> >                 */
> 
> 'int' is not necessarily 4 bytes; I would feel slightly more
> comfortable had upstream used int32_t.  I know you are just copying
> code from an existing header (so don't change the struct itself),
> but wonder if we should at least add our own sanity checking:
> 
> verify(sizeof(virInitctlRequest)) == 384);

I'm just adding the verify, since I think that's the more
important thing todo.

> 
> > +
> > +    if ((fd = open(path, O_WRONLY|O_NDELAY|O_CLOEXEC|O_NOCTTY)) < 0)
> 
> O_NDELAY is non-standard.  I would spell it according to POSIX as
> O_NONBLOCK.

Yep, good point.

> > +typedef enum virInitctlRunLevel virInitctlRunLevel;
> > +enum virInitctlRunLevel {
> > +    VIR_INITCTL_RUNLEVEL_POWEROFF = 0,
> > +    VIR_INITCTL_RUNLEVEL_1 = 1,
> > +    VIR_INITCTL_RUNLEVEL_2 = 2,
> > +    VIR_INITCTL_RUNLEVEL_3 = 3,
> > +    VIR_INITCTL_RUNLEVEL_4 = 4,
> > +    VIR_INITCTL_RUNLEVEL_5 = 5,
> > +    VIR_INITCTL_RUNLEVEL_REBOOT = 6,
> 
> Should you add VIR_INITCTL_RUNLEVEL_LAST, in case we ever
> expand this list?

Unlikely, but doesn't hurt to have a sentinal

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|