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

[Stefano Brivio <sbrivio@redhat.com>]

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