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