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

On Mon, Jun 17, 2019 at 9:52 AM Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > 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 I'm up for option c with 1024 as a limit. Tbh, I think the specific limit value is not super important neither we should spend much more time thinking about it. So I'll send a new patch with 1024 as the limit and an update of the docs.