Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal

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&gt; 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