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

On Fri, Jun 14, 2019 at 12:59:11PM +0200, Ilias Stamatis wrote: > On Fri, Jun 14, 2019 at 10:07 AM Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > > > 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 > > Of course I meant 2^64, but I had 8 bytes in my mind so I got confused. > > This scenario you describe makes sense, even though currently only the > LXC driver implements this API. Yes, and I sincerely doubt any other hypervisor would ever implement this since this should completely in the guest OS' scope, e.g. in case of QEMU, most likely qemu-guest-agent would have to be involved, which means that good old ssh will work just as nicely + you'd more probably want to terminate services by their name rather than pid (and even more probably you want to terminate service units, not individual pids). > > So in conclusion I would say that the possible options are a) only > succeed for pid 1 b) succeed for ANY pid c) succeed for any pid up to > a pid_limit. a) using this for test driver is okay, because it doesn't do anything, but then again, from logical POV, it doesn't make any sense to me, init ignores/masks almost all the signals, even if it didn't, the user most likely just panicked the guest's kernel, great, virDomainDestroy would have served much better here. b) phrdina complained that we lose the error path if we succeed for every PID and that's true, but gives more flexibility c) I guess I'd personally prefer this one, but what should the arbitrary limit be? (e.g. for < 1024 and succeed otherwise, dunno...) Whichever option we choose, we need to clarify it very clearly in the documentation, so I'll leave the choice to you as long as there's a doc string documenting the behaviour of the API in v2. Erik