Re: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
On 4/21/20 8:10 PM, Marc-André Lureau wrote: Oh ok, that should be fine. thanks Cool! I've made the change and pushed. To all: Reviewed-by: Michal Privoznik Michal
Re: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
Hi On Tue, Apr 21, 2020 at 7:42 PM Michal Privoznik wrote: > > On 4/21/20 6:50 PM, Marc-André Lureau wrote: > > Hi > > > > On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik > > wrote: > >> > >> On 4/8/20 7:23 PM, marcandre.lur...@redhat.com wrote: > >>> From: Marc-André Lureau > >>> > >>> Don't stop the DBus daemon if a slirp helper failed to start, as it > >>> may be shared with other helpers. > >>> > >>> Signed-off-by: Marc-André Lureau > >>> --- > >>>src/qemu/qemu_slirp.c | 2 +- > >>>1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c > >>> index 09c1247892..49bffa01b8 100644 > >>> --- a/src/qemu/qemu_slirp.c > >>> +++ b/src/qemu/qemu_slirp.c > >>> @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, > >>>virProcessKillPainfully(pid, true); > >>>if (pidfile) > >>>unlink(pidfile); > >>> -qemuDBusStop(driver, vm); > >>> +/* leave dbus daemon running, it may be used by others */ > >>>return -1; > >>>} > >>> > >> > >> I'm not quite sure about this one. Who do you mean by "others"? Other > > > > Other users of DBus. For now, it's only slirp-helper, but there can be > > already multiple instances. > > Ah, I've misunderstood this part. But if there is only one user of the > DBus (us who are trying to start it now), shouldn't we kill the dbus > daemon? On the other hand, it will be done by qemuProcessStop() > eventually. My idea was to track whether the dbus daemon is running > prior starting it (basically save priv->dbusDaemonRunning before calling > qemuDBusStart() and then wrap this qemuDBusStop() with 'if > (was_started)'. I can do the change before push, if you are okay with > it. I just like functions to tidy up on failure, maybe it's just an > obsession though. Oh ok, that should be fine. thanks
Re: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
On 4/21/20 6:50 PM, Marc-André Lureau wrote: Hi On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik wrote: On 4/8/20 7:23 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Don't stop the DBus daemon if a slirp helper failed to start, as it may be shared with other helpers. Signed-off-by: Marc-André Lureau --- src/qemu/qemu_slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 09c1247892..49bffa01b8 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); -qemuDBusStop(driver, vm); +/* leave dbus daemon running, it may be used by others */ return -1; } I'm not quite sure about this one. Who do you mean by "others"? Other Other users of DBus. For now, it's only slirp-helper, but there can be already multiple instances. Ah, I've misunderstood this part. But if there is only one user of the DBus (us who are trying to start it now), shouldn't we kill the dbus daemon? On the other hand, it will be done by qemuProcessStop() eventually. My idea was to track whether the dbus daemon is running prior starting it (basically save priv->dbusDaemonRunning before calling qemuDBusStart() and then wrap this qemuDBusStop() with 'if (was_started)'. I can do the change before push, if you are okay with it. I just like functions to tidy up on failure, maybe it's just an obsession though. Michal
Re: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
Hi On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik wrote: > > On 4/8/20 7:23 PM, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Don't stop the DBus daemon if a slirp helper failed to start, as it > > may be shared with other helpers. > > > > Signed-off-by: Marc-André Lureau > > --- > > src/qemu/qemu_slirp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c > > index 09c1247892..49bffa01b8 100644 > > --- a/src/qemu/qemu_slirp.c > > +++ b/src/qemu/qemu_slirp.c > > @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, > > virProcessKillPainfully(pid, true); > > if (pidfile) > > unlink(pidfile); > > -qemuDBusStop(driver, vm); > > +/* leave dbus daemon running, it may be used by others */ > > return -1; > > } > > > > I'm not quite sure about this one. Who do you mean by "others"? Other Other users of DBus. For now, it's only slirp-helper, but there can be already multiple instances. > interfaces? Is this supposed to help with 3/6 so that if we attempt to > double start the dbus daemon the second attempt doesn't actually kill > the daemon started in the first attempt? yes, the point is to have a single bus for the VM needs. thanks > > ACK to the rest. > > Michal >
Re: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
On 4/8/20 7:23 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Don't stop the DBus daemon if a slirp helper failed to start, as it may be shared with other helpers. Signed-off-by: Marc-André Lureau --- src/qemu/qemu_slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 09c1247892..49bffa01b8 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); -qemuDBusStop(driver, vm); +/* leave dbus daemon running, it may be used by others */ return -1; } I'm not quite sure about this one. Who do you mean by "others"? Other interfaces? Is this supposed to help with 3/6 so that if we attempt to double start the dbus daemon the second attempt doesn't actually kill the daemon started in the first attempt? ACK to the rest. Michal
