qemu_passt: Fix issues with PID file

[PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

Posted by Michal Privoznik 2 years, 11 months ago

There are two places where we kill passt:

1) qemuPasstStop() - called transitively from qemuProcessStop(),
2) qemuPasstStart() - after failed start.

Now, the code from 2) lack error preservation (so if there's
another error during cleanup we might overwrite the original
error). Therefore, move the internals of qemuPasstStop() into a
separate function and call it from both places.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_passt.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index 881205449b..a4cc9e7166 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
 }
 
 
-void
-qemuPasstStop(virDomainObj *vm,
-              virDomainNetDef *net)
+static void
+qemuPasstKill(const char *pidfile)
 {
-    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
     virErrorPtr orig_err;
 
     virErrorPreserveLast(&orig_err);
@@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
 }
 
 
+void
+qemuPasstStop(virDomainObj *vm,
+              virDomainNetDef *net)
+{
+    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
+
+    qemuPasstKill(pidfile);
+}
+
+
 int
 qemuPasstSetupCgroup(virDomainObj *vm,
                      virDomainNetDef *net,
@@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
     g_autofree char *errbuf = NULL;
     char macaddr[VIR_MAC_STRING_BUFLEN];
     size_t i;
-    pid_t pid = (pid_t) -1;
     int exitstatus = 0;
     int cmdret = 0;
 
@@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
     return 0;
 
  error:
-    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
-    if (pid != -1)
-        virProcessKillPainfully(pid, true);
-    unlink(pidfile);
-
+    qemuPasstKill(pidfile);
     return -1;
 }
-- 
2.39.1

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

Posted by Laine Stump 2 years, 11 months ago

On 2/16/23 8:32 AM, Michal Privoznik wrote:
> There are two places where we kill passt:
> 
> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
> 2) qemuPasstStart() - after failed start.
> 
> Now, the code from 2) lack error preservation (so if there's
> another error during cleanup we might overwrite the original
> error). Therefore, move the internals of qemuPasstStop() into a
> separate function and call it from both places.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

> ---
>   src/qemu/qemu_passt.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 881205449b..a4cc9e7166 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
>   }
>   
>   
> -void
> -qemuPasstStop(virDomainObj *vm,
> -              virDomainNetDef *net)
> +static void
> +qemuPasstKill(const char *pidfile)
>   {
> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>       virErrorPtr orig_err;
>   
>       virErrorPreserveLast(&orig_err);
> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
>   }
>   
>   
> +void
> +qemuPasstStop(virDomainObj *vm,
> +              virDomainNetDef *net)
> +{
> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> +
> +    qemuPasstKill(pidfile);
> +}
> +
> +
>   int
>   qemuPasstSetupCgroup(virDomainObj *vm,
>                        virDomainNetDef *net,
> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
>       g_autofree char *errbuf = NULL;
>       char macaddr[VIR_MAC_STRING_BUFLEN];
>       size_t i;
> -    pid_t pid = (pid_t) -1;
>       int exitstatus = 0;
>       int cmdret = 0;
>   
> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
>       return 0;
>   
>    error:
> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
> -    if (pid != -1)
> -        virProcessKillPainfully(pid, true);
> -    unlink(pidfile);
> -
> +    qemuPasstKill(pidfile);
>       return -1;
>   }

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

Posted by Stefano Brivio 2 years, 11 months ago

On Thu, 16 Feb 2023 14:32:51 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> There are two places where we kill passt:
> 
> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
> 2) qemuPasstStart() - after failed start.
> 
> Now, the code from 2) lack error preservation (so if there's
> another error during cleanup we might overwrite the original
> error). Therefore, move the internals of qemuPasstStop() into a
> separate function and call it from both places.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_passt.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 881205449b..a4cc9e7166 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
>  }
>  
>  
> -void
> -qemuPasstStop(virDomainObj *vm,
> -              virDomainNetDef *net)
> +static void
> +qemuPasstKill(const char *pidfile)

A minor comment, should you respin: I think it should be made clear that
this is not the expected/normal way in which passt will terminate --
here or in the next patch. Removing the PID file is nice, but that's
(usually) about it.

>  {
> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>      virErrorPtr orig_err;
>  
>      virErrorPreserveLast(&orig_err);
> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
>  }
>  
>  
> +void
> +qemuPasstStop(virDomainObj *vm,
> +              virDomainNetDef *net)
> +{
> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> +
> +    qemuPasstKill(pidfile);
> +}
> +
> +
>  int
>  qemuPasstSetupCgroup(virDomainObj *vm,
>                       virDomainNetDef *net,
> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
>      g_autofree char *errbuf = NULL;
>      char macaddr[VIR_MAC_STRING_BUFLEN];
>      size_t i;
> -    pid_t pid = (pid_t) -1;
>      int exitstatus = 0;
>      int cmdret = 0;
>  
> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
>      return 0;
>  
>   error:
> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
> -    if (pid != -1)
> -        virProcessKillPainfully(pid, true);
> -    unlink(pidfile);
> -
> +    qemuPasstKill(pidfile);

...what takes care of terminating passt in case qemu doesn't start, now?
The fact that the process is in the cgroup, right?

>      return -1;
>  }

-- 
Stefano

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

Posted by Michal Prívozník 2 years, 11 months ago

On 2/16/23 17:07, Stefano Brivio wrote:
> On Thu, 16 Feb 2023 14:32:51 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> There are two places where we kill passt:
>>
>> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
>> 2) qemuPasstStart() - after failed start.
>>
>> Now, the code from 2) lack error preservation (so if there's
>> another error during cleanup we might overwrite the original
>> error). Therefore, move the internals of qemuPasstStop() into a
>> separate function and call it from both places.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_passt.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
>> index 881205449b..a4cc9e7166 100644
>> --- a/src/qemu/qemu_passt.c
>> +++ b/src/qemu/qemu_passt.c
>> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
>>  }
>>  
>>  
>> -void
>> -qemuPasstStop(virDomainObj *vm,
>> -              virDomainNetDef *net)
>> +static void
>> +qemuPasstKill(const char *pidfile)
> 
> A minor comment, should you respin: I think it should be made clear that
> this is not the expected/normal way in which passt will terminate --
> here or in the next patch. Removing the PID file is nice, but that's
> (usually) about it.


I can adjust the commit message.

> 
>>  {
>> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>>      virErrorPtr orig_err;
>>  
>>      virErrorPreserveLast(&orig_err);
>> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
>>  }
>>  
>>  
>> +void
>> +qemuPasstStop(virDomainObj *vm,
>> +              virDomainNetDef *net)
>> +{
>> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>> +
>> +    qemuPasstKill(pidfile);
>> +}
>> +
>> +
>>  int
>>  qemuPasstSetupCgroup(virDomainObj *vm,
>>                       virDomainNetDef *net,
>> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
>>      g_autofree char *errbuf = NULL;
>>      char macaddr[VIR_MAC_STRING_BUFLEN];
>>      size_t i;
>> -    pid_t pid = (pid_t) -1;
>>      int exitstatus = 0;
>>      int cmdret = 0;
>>  
>> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
>>      return 0;
>>  
>>   error:
>> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
>> -    if (pid != -1)
>> -        virProcessKillPainfully(pid, true);
>> -    unlink(pidfile);
>> -
>> +    qemuPasstKill(pidfile);
> 
> ...what takes care of terminating passt in case qemu doesn't start, now?
> The fact that the process is in the cgroup, right?

No, it's qemuPasstKill(). Starting a guest is done in
qemuProcessStart(). In here, qemuProcessLaunch() ->
qemuExtDevicesStart() -> qemuPasstStart() is called. Now, in the top
most parent (qemuProcessStart()) - you can see the 'stop' label in which
qemuProcessStop() -> qemuExtDevicesStop() -> qemuPasstStop() is called.

We could go the extra step and use either virCgroupKillRecursive() or
virCgroupKillPainfully() to kill all processes within the CGroup. BUT,
the driver runs on different OSes than Linux (e.g. FreeBSD) or on Linux
with no CGroups.

Michal

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

Posted by Stefano Brivio 2 years, 11 months ago

On Thu, 16 Feb 2023 17:38:47 +0100
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 2/16/23 17:07, Stefano Brivio wrote:
> > On Thu, 16 Feb 2023 14:32:51 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> There are two places where we kill passt:
> >>
> >> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
> >> 2) qemuPasstStart() - after failed start.
> >>
> >> Now, the code from 2) lack error preservation (so if there's
> >> another error during cleanup we might overwrite the original
> >> error). Therefore, move the internals of qemuPasstStop() into a
> >> separate function and call it from both places.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_passt.c | 23 +++++++++++++----------
> >>  1 file changed, 13 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> >> index 881205449b..a4cc9e7166 100644
> >> --- a/src/qemu/qemu_passt.c
> >> +++ b/src/qemu/qemu_passt.c
> >> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
> >>  }
> >>  
> >>  
> >> -void
> >> -qemuPasstStop(virDomainObj *vm,
> >> -              virDomainNetDef *net)
> >> +static void
> >> +qemuPasstKill(const char *pidfile)  
> > 
> > A minor comment, should you respin: I think it should be made clear that
> > this is not the expected/normal way in which passt will terminate --
> > here or in the next patch. Removing the PID file is nice, but that's
> > (usually) about it.  
> 
> I can adjust the commit message.

Okay, I'm not really familiar with libvirt's code, so I don't know how
appropriate this is -- I was just suggesting that a _comment_ to a
qemuPasstKill() function which does *not* actually kill the passt
process, unless an error occurs, wouldn't look so bizarre.

> >>  {
> >> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> >>      virErrorPtr orig_err;
> >>  
> >>      virErrorPreserveLast(&orig_err);
> >> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
> >>  }
> >>  
> >>  
> >> +void
> >> +qemuPasstStop(virDomainObj *vm,
> >> +              virDomainNetDef *net)
> >> +{
> >> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> >> +
> >> +    qemuPasstKill(pidfile);
> >> +}
> >> +
> >> +
> >>  int
> >>  qemuPasstSetupCgroup(virDomainObj *vm,
> >>                       virDomainNetDef *net,
> >> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
> >>      g_autofree char *errbuf = NULL;
> >>      char macaddr[VIR_MAC_STRING_BUFLEN];
> >>      size_t i;
> >> -    pid_t pid = (pid_t) -1;
> >>      int exitstatus = 0;
> >>      int cmdret = 0;
> >>  
> >> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
> >>      return 0;
> >>  
> >>   error:
> >> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
> >> -    if (pid != -1)
> >> -        virProcessKillPainfully(pid, true);
> >> -    unlink(pidfile);
> >> -
> >> +    qemuPasstKill(pidfile);  
> > 
> > ...what takes care of terminating passt in case qemu doesn't start, now?
> > The fact that the process is in the cgroup, right?  
> 
> No, it's qemuPasstKill(). Starting a guest is done in
> qemuProcessStart(). In here, qemuProcessLaunch() ->
> qemuExtDevicesStart() -> qemuPasstStart() is called. Now, in the top
> most parent (qemuProcessStart()) - you can see the 'stop' label in which
> qemuProcessStop() -> qemuExtDevicesStop() -> qemuPasstStop() is called.

Ah, okay, thanks for the explanation, I see now (well, it makes
sense with 5/5).

-- 
Stefano