[libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
From man 2 kill: The only signals that can be sent to process ID 1, the init process, are
Only succeed when @pid_value is 1, since according to the docs this is the minimum requirement for any driver to implement this API. 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@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
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 With the check removed: Reviewed-by: Erik Skultety <eskultet@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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jun 13, 2019 at 10:20 AM Erik Skultety <eskultet@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.
With the check removed: Reviewed-by: Erik Skultety <eskultet@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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jun 14, 2019 at 10:07 AM Erik Skultety <eskultet@redhat.com> 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@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
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. 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. All 3 options make sense to me. Maybe I would be a bit more in favor of the second one since choosing a pid limit would be a bit arbitrary. Ilias
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@redhat.com> 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@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
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
On Mon, Jun 17, 2019 at 9:52 AM Erik Skultety <eskultet@redhat.com> 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@redhat.com> 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@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
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. Ilias
On Mon, Jun 17, 2019 at 10:14:37PM +0200, Ilias Stamatis wrote:
On Mon, Jun 17, 2019 at 9:52 AM Erik Skultety <eskultet@redhat.com> 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@redhat.com> 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@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
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.
I think that even this limit is pointless, it will be most likely used only in tests to validate that you can integrate with libvirt and you can handle both cases when it fails or succeed and the specific PID is not relevant, but feel free to implement the 1024 limit as well if it's properly documented. Pavel
On Tue, Jun 18, 2019 at 03:43:08PM +0200, Pavel Hrdina wrote:
On Mon, Jun 17, 2019 at 10:14:37PM +0200, Ilias Stamatis wrote:
On Mon, Jun 17, 2019 at 9:52 AM Erik Skultety <eskultet@redhat.com> 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@redhat.com> 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@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
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.
I think that even this limit is pointless, it will be most likely used only in tests to validate that you can integrate with libvirt and you can handle both cases when it fails or succeed and the specific PID is not relevant, but feel free to implement the 1024 limit as well if it's properly documented.
Pavel
Fair enough. I'll merge the original patch then. Erik
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 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.
I don't thing the previous 2 paragraphs are needed, I think that stating "Since this is test driver, we assume that any signal from the supported list can be sent to pid 1 and we therefore succeed every time" is sufficient. Reviewed-by: Erik Skultety <eskultet@redhat.com>
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 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@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"));
^This fails make syntax-check, since % format specifier is always required, in this case "%s". I'll fix that before merging. Erik
participants (3)
-
Erik Skultety -
Ilias Stamatis -
Pavel Hrdina