[libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal

Ilias Stamatis posted 1 patch 4 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190604131743.17204-1-stamatis.iliass@gmail.com
src/test/test_driver.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
mode change 100644 => 100755 src/test/test_driver.c
[libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Ilias Stamatis 4 years, 9 months ago
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"));
+        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
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Erik Skultety 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Ilias Stamatis 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Erik Skultety 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Ilias Stamatis 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Erik Skultety 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Ilias Stamatis 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Pavel Hrdina 4 years, 9 months ago
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Erik Skultety 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Erik Skultety 4 years, 9 months ago
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>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainSendProcessSignal
Posted by Erik Skultety 4 years, 9 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list