[libvirt] [PATCH 1/4] Add virDomainSendProcessSignal API

[Eric Blake]

> Add an API for sending signals to arbitrary processes in the
> guest OS. This is primarily useful for container based virt,
> but can be used for machine virt too, if there is a suitable
> guest agent,
> 
> * include/libvirt/libvirt.h.in: Add virDomainSendProcessSignal
>   and virDomainProcessSignal enum
> * src/driver.h: Driver entry point
> * src/libvirt.c, src/libvirt_public.syms: Impl for new API
> ---
>  
>  /*
> + * These just happen to match Linux signal numbers. The numbers
> + * will be mapped to whatever the SIGNUM is in the guest OS in
> + * question by the agent delivering the signal. The names are
> + * based on the POSIX / XSI signal standard though.
> + *
> + * Values over VIR_DOMAIN_PROCESS_SIGNAL_RTMIN are allowed,
> + * and will be mapped to the corresponding offset of the
> + * OS specific SIGRTMIN value

> +    VIR_DOMAIN_PROCESS_SIGNAL_SYS        = 31, /* SIGSYS (also known
> as SIGUNUSED on Linux ) */
> +
> +    VIR_DOMAIN_PROCESS_SIGNAL_RTMIN      = 32, /* SIGRTMIN */
> +
> +#ifdef VIR_ENUM_SENTINELS
> +    /* This is the "last" signal only in so much as none
> +     * of the latter ones have named constants. They are
> +     * just numeric offsets from RTMIN upwards
> +     */
> +    VIR_DOMAIN_PROCESS_SIGNAL_LAST
> +#endif
> +} virDomainProcessSignal;

I'm not comfortable with this; there are only a bounded number of signals
allowed over SIGRTMIN (namely, up to SIGRTMAX); POSIX requires at least
8 real-time signals, but at the moment, Linux has 32 and 32-bit cygwin
has only 1 (work is underway on building 64-bit cygwin which will copy
Linux in having 32 real-time signals).

The problem with having an unbounded number of realtime signals as the
end of our list is that we cannot ever add extensions for any other
signals on non-Linux hosts.  We are unlikely to hit any implementation
with more than 64 named signal values.  Maybe we just bite the bullet
and call out 32 VIR_DOMAIN_PROCESS_SIGNAL_REALTIME_NNN values, at which
point, the VIR_DOMAIN_PROCESS_SIGNAL_LAST sentinel really is a sentinel
and we are free to add more enums later if porting to a different platform.

The other thing I'm worried about is that you didn't document what happens
if a user passes a VIR_DOMAIN_PROCESS_SIGNAL_ value that we can't map
to the underlying implementation (of course, it won't happen for Linux
to Linux; but again, if we extend this to other platforms, then it is
very reasonable to expect that VIR_DOMAIN_PROCESS_SIGNAL_STKFLT must
be rejected if the guest OS doesn't have a STKFLT signal, or that realtime
signals must fit within the bounds of the guest OS, even if those bounds
differ from the Linux default of 32 levels).

> +int virDomainSendProcessSignal(virDomainPtr domain,
> +                               unsigned int pid_value,

Do we really want 'unsigned int' pid_value, or do we want to
allow the user to send signals to process groups (negative
pid_t)?  Also, should we make the parameter 'long long int'
in order to accomodate 64-bit Windows having 64-bit pid_t?

> +typedef int
> +    (*virDrvDomainSendProcessSignal)(virDomainPtr dom,
> +                                     unsigned int pid_value,
> +                                     unsigned int signum,
> +                                     unsigned int flags);

Obviously, this has to reflect any type changes made in the public API.

> +/**
> + * virDomainSendProcessSignal:
> + * @domain: pointer to domain object
> + * @pid_value: a positive integer process ID greater than zero

Why greater than zero?  Is there any reason to forbid killing a
process group?

> + * @signum: a signal from the virDomainProcessSignal enum
> + * @flags: one of the virDomainProcessSignalFlag values
> + *
> + * Send a signal to the designated process in the guest
> + *
> + * The signal numbers must be taken from the virDomainProcessSignal
> + * enum. These will be translated to the corresponding signal
> + * number for the guest OS, by the guest agent delivering the
> + * signal.
> + *
> + * The @pid_value must specify a process ID greater than zero. The
> + * @pid_value numbers are from the container/guest namespace.

Maybe document that some hypervisors restrict which @pid_value can
be used, to cover the fact that your initial LXC implementation
requires @pid_value of 1.

> +++ b/src/libvirt_public.syms
> @@ -574,4 +574,9 @@ LIBVIRT_1.0.0 {
>          virNodeGetCPUMap;
>  } LIBVIRT_0.10.2;
>  
> +LIBVIRT_1.0.1 {
> +    global:
> +        virDomainSendProcessSignal;

Potential merge conflicts here, but that's nothing too hard.

I'm generally in favor of a new API along these lines, but am
worried that your initial implementation may be painting us
into portability corners.