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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list