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

[Stefano Brivio <sbrivio@redhat.com>]

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