[libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal

[Erik Skultety]

On Thu, Jun 13, 2019 at 02:20:22PM +0200, Ilias Stamatis wrote:
> On Thu, Jun 13, 2019 at 10:20 AM Erik Skultety <eskultet at redhat.com> wrote:
> >
> > On Tue, Jun 04, 2019 at 03:17:43PM +0200, Ilias Stamatis wrote:
> > > Only succeed when @pid_value is 1, since according to the docs this is
> >
> > Why do we need to restrict ourselves for @pid 1 in the test driver? This
> > restriction exists in LXC for a reason, but why in test driver?
> > a) it's only a check with @pid not being actually used
> > b) each guest OS will have their own limit in /proc/sys/kernel/pid_max for the
> > max number of PID so restricting it from the top doesn't make sense
> > c) - at pid is used for process groups, so again, this will be handled within the
> > guest OS in reality
>
> To my understanding if there's no process with such pid in the guest,
> this API will fail. If we succeed for any pid, that would mean that
> the test domain has 2^8 processes running and I wasn't sure if we
> wanted that. But yes, as I wrote as a "comment" in the initial patch
> this could as well make sense within test driver's scope so it's fine
> to remove the check.

Well, 2^8 isn't that bad is it? That's realistic, but long long is 64bit, so
2^63-1 that's a lot of PIDs, so you're right, it would feel weird, on the
other hand, looking at it from the perspective of an app developer, requiring
pid==1 is restrictive, if you allow any pid, you give them flexibility - PID
being filled from a dynamic data structure, so eventually, they're just simply
switch test driver for the real API, but that is my perspective, I don't want
to force it to upstream, we can wait for another opinion, one can say that if
they want to test with dynamic PIDs, use the real API.

Erik

>
> >
> > With the check removed:
> > Reviewed-by: Erik Skultety <eskultet at redhat.com>
> >
> > > the minimum requirement for any driver to implement this API.
> > >
> > > >From man 2 kill:
> > > The only signals that can be sent to process ID 1, the init process, are
> > > those for which init has explicitly installed signal handlers.
> > >
> > > Regarding sending SIGTERM and SIGKILL to init, the test driver domains
> > > pretend to run Linux, and on Linux init/systemd explicitly ignores these
> > > signals.
> > >
> > > Correspondingly, we can assume that no signal handlers are installed for
> > > any other signal and succeed by doing nothing.
> > >
> > > Signed-off-by: Ilias Stamatis <stamatis.iliass at gmail.com>
> > > ---
> > >
> > > Alternatively, we could succeed when @pid_value is any number or belongs
> > > to a set of specific numbers.
> > >
> > >  src/test/test_driver.c | 35 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >  mode change 100644 => 100755 src/test/test_driver.c
> > >
> > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > old mode 100644
> > > new mode 100755
> > > index cae2521b21..490a605a77
> > > --- a/src/test/test_driver.c
> > > +++ b/src/test/test_driver.c
> > > @@ -2825,6 +2825,40 @@ static int testDomainSetMetadata(virDomainPtr dom,
> > >      return ret;
> > >  }
> > >
> > > +static int
> > > +testDomainSendProcessSignal(virDomainPtr dom,
> > > +                            long long pid_value,
> > > +                            unsigned int signum,
> > > +                            unsigned int flags)
> > > +{
> > > +    int ret = -1;
> > > +    virDomainObjPtr vm = NULL;
> > > +
> > > +    virCheckFlags(0, -1);
> > > +
> > > +    if (pid_value != 1) {
> > > +        virReportError(VIR_ERR_INVALID_ARG,
> > > +                       _("only sending a signal to pid 1 is supported"));
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (signum >= VIR_DOMAIN_PROCESS_SIGNAL_LAST) {
> > > +        virReportError(VIR_ERR_INVALID_ARG,
> > > +                       _("signum value %d is out of range"),
> > > +                       signum);
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (!(vm = testDomObjFromDomain(dom)))
> > > +        goto cleanup;
> > > +
> > > +    /* do nothing */
> > > +    ret = 0;
> > > +
> > > + cleanup:
> > > +    virDomainObjEndAPI(&vm);
> > > +    return ret;
> > > +}
> > >
> > >  static int testNodeGetCellsFreeMemory(virConnectPtr conn,
> > >                                        unsigned long long *freemems,
> > > @@ -7027,6 +7061,7 @@ static virHypervisorDriver testHypervisorDriver = {
> > >      .domainScreenshot = testDomainScreenshot, /* 1.0.5 */
> > >      .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
> > >      .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
> > > +    .domainSendProcessSignal = testDomainSendProcessSignal, /* 5.5.0 */
> > >      .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */
> > >      .domainManagedSave = testDomainManagedSave, /* 1.1.4 */
> > >      .domainHasManagedSaveImage = testDomainHasManagedSaveImage, /* 1.1.4 */
> > > --
> > > 2.21.0
> > >
> > > --
> > > libvir-list mailing list
> > > libvir-list at redhat.com
> > > https://www.redhat.com/mailman/listinfo/libvir-list