3:07 a.m.
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(a)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) -@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(a)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(a)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(a)redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list