On Thu, 16 Feb 2023 17:38:47 +0100
Michal Prívozník <mprivozn@xxxxxxxxxx> wrote:
> On 2/16/23 17:07, Stefano Brivio wrote:
> > On Thu, 16 Feb 2023 14:32:51 +0100
> > Michal Privoznik <mprivozn@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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
