[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/4] Add virDomainSendProcessSignal API



> 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
> ---
>  include/libvirt/libvirt.h.in | 97
>  ++++++++++++++++++++++++++++++++++++++++++++
>  src/driver.h                 |  7 ++++
>  src/libvirt.c                | 84
>  ++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms      |  1 +
>  4 files changed, 189 insertions(+)

> + *
> + * Do not rely on all values  matching Linux though. It is possible

Why two spaces mid-sentence?

> +    VIR_DOMAIN_PROCESS_SIGNAL_RT32       = 64, /* SIGRTMIN + 32 /
> SIGRTMAX */

Yes, this works better for me.

> +
> +#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

This comment is now out of date.

> +/**
> + * virDomainSendProcessSignal:
> + * @domain: pointer to domain object
> + * @pid_value: a positive integer process ID, or negative integer
> process group ID
> + * @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. If there is no mapping from virDomainProcessSignal to
> + * the native OS signals, this API will report an error

s/$/./

> + *
> + * If @pid_value is an integer greater than zero, it is
> + * treated as a process ID. If @pid_value is an integer
> + * less than zero, it is treated as a process group ID.
> + * All the @pid_value numbers are from the container/guest
> + * namespace. The value zero is not valid.
> + *
> + * Not all hypervisors will support sending signals to
> + * arbitrary processes or process groups. If this API is
> + * implemented the minimum requirement is to be able to
> + * use @pid_value==1 (ie kill init). No other value is

s/ie/i.e./

> +
> +    if (signum > VIR_DOMAIN_PROCESS_SIGNAL_LAST) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }

Drop this hunk (rather, move it to patch 4/4).  Such a check
should be done in the hypervisor driver, not in the entry
point.  Otherwise, if we add a new signal later, this hunk
would prevent an older virsh from sending the new signal.

ACK with those changes.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]