Re: [PATCH 3/8] qemu-conf: add dbusStateDir
On a Friday in 2020, Marc-André Lureau wrote: Hi On Tue, Feb 25, 2020 at 12:24 PM Michal Privoznik wrote: On 2/24/20 4:57 PM, Marc-André Lureau wrote: > Hi > > On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik wrote: >> >> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote: >>> From: Marc-André Lureau >>> >>> Location of DBus daemon state configuration, socket, pid... >>> >>> Signed-off-by: Marc-André Lureau >>> --- >>>src/qemu/qemu_conf.c | 4 >>>src/qemu/qemu_conf.h | 1 + >>>2 files changed, 5 insertions(+) >>> >>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c >>> index e1fea390c7..ade12dac6c 100644 >>> --- a/src/qemu/qemu_conf.c >>> +++ b/src/qemu/qemu_conf.c >>> @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) >>> >>>cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR); >>> >>> +cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); >>> + >>>cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); >>>cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); >>>cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); >>> @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) >>>cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); >>> >>>cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); >>> +cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); >>> >>>cfg->configBaseDir = virGetUserConfigDirectory(); >> >> Instead of doing practically the same in two branches, you can achieve >> the same result with just one line if you put the call just below >> cfg->slirpStateDir init. >> >> However, do we need this to be in a special directory instead of per VM >> private directory? The way I implemented PR helper was that the socket >> it creates and to which qemu connects is stored under vm->priv->libDir >> which is initialized in qemuDomainSetPrivatePaths() to: >> >> $cfg->libDir/domain-$shortName/ >> >> This way you wouldn't need to care about domain's shortname in the next >> patch. > > Makes sense. I think I based this on slirpStateDir code. Any reason > it's not using vm->priv->libdir too? > I don't know. But since there are some releases which would store data under slirpStateDir I don't think we can change this. On daemon restart (with newer version) the new libvirtd wouldn't see the old dir. Actually, $cfg->libDir is virQEMUDriverConfigNew () cfg->configBaseDir/qemu/lib, so it ends up under ~/.config for users, and /etc for priviledged/root. A bad place to store transient runtime For the privileged driver, cfg->libDir is derived from LOCALSTATEDIR, so it goes in /var. Using ~/.config for unprivileged libDir does sound like an odd choice, but if that's so we should fix it for all libDir users, not keep introducing new Dirs. Jano data/sockets. You may want to reconsider the paths for pr-helper. I don't intend to change that, so you could take a look at "[libvirt PATCH v2 0/9]". signature.asc Description: PGP signature
Re: [PATCH 3/8] qemu-conf: add dbusStateDir
On 2/25/20 6:24 AM, Michal Privoznik wrote: On 2/24/20 4:57 PM, Marc-André Lureau wrote: Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik wrote: On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Location of DBus daemon state configuration, socket, pid... Signed-off-by: Marc-André Lureau --- src/qemu/qemu_conf.c | 4 src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR); + cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); cfg->configBaseDir = virGetUserConfigDirectory(); Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init. However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to: $cfg->libDir/domain-$shortName/ This way you wouldn't need to care about domain's shortname in the next patch. Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too? I don't know. But since there are some releases which would store data under slirpStateDir I don't think we can change this. On daemon restart (with newer version) the new libvirtd wouldn't see the old dir. It would need to be done carefully, but it's not out of the question: There was previously a case where we had been storing the network state files in an incorrect location (/var/lib/libvirt) (starting in commit 23a090ab92 - libvirt-0.6.0 in 2009), and moved them to a more proper location (/var/run/libvirt) (commit b9e95491d1 - libvirt 1.2.4 in 2014). At the time the move was made, we also added a code to networkStateInitialize() that looked for files in the old location and moved them to the new location. This code remained in place, at first doing its job whenever someone upgraded, and then silently doing nothing for a very long time, until 5 years later when it was removed in commit 43be65a4817f - libvirt 5.1.0. (probably could have been removed much earlier than that, but it had just been forgotten.)
Re: [PATCH 3/8] qemu-conf: add dbusStateDir
Hi On Tue, Feb 25, 2020 at 12:24 PM Michal Privoznik wrote: > > On 2/24/20 4:57 PM, Marc-André Lureau wrote: > > Hi > > > > On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik > > wrote: > >> > >> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote: > >>> From: Marc-André Lureau > >>> > >>> Location of DBus daemon state configuration, socket, pid... > >>> > >>> Signed-off-by: Marc-André Lureau > >>> --- > >>>src/qemu/qemu_conf.c | 4 > >>>src/qemu/qemu_conf.h | 1 + > >>>2 files changed, 5 insertions(+) > >>> > >>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > >>> index e1fea390c7..ade12dac6c 100644 > >>> --- a/src/qemu/qemu_conf.c > >>> +++ b/src/qemu/qemu_conf.c > >>> @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool > >>> privileged) > >>> > >>>cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", > >>> LOCALSTATEDIR); > >>> > >>> +cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", > >>> LOCALSTATEDIR); > >>> + > >>>cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", > >>> LOCALSTATEDIR); > >>>cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); > >>>cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); > >>> @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool > >>> privileged) > >>>cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); > >>> > >>>cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", > >>> cfg->stateDir); > >>> +cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); > >>> > >>>cfg->configBaseDir = virGetUserConfigDirectory(); > >> > >> Instead of doing practically the same in two branches, you can achieve > >> the same result with just one line if you put the call just below > >> cfg->slirpStateDir init. > >> > >> However, do we need this to be in a special directory instead of per VM > >> private directory? The way I implemented PR helper was that the socket > >> it creates and to which qemu connects is stored under vm->priv->libDir > >> which is initialized in qemuDomainSetPrivatePaths() to: > >> > >> $cfg->libDir/domain-$shortName/ > >> > >> This way you wouldn't need to care about domain's shortname in the next > >> patch. > > > > Makes sense. I think I based this on slirpStateDir code. Any reason > > it's not using vm->priv->libdir too? > > > > I don't know. But since there are some releases which would store data > under slirpStateDir I don't think we can change this. On daemon restart > (with newer version) the new libvirtd wouldn't see the old dir. Actually, $cfg->libDir is virQEMUDriverConfigNew () cfg->configBaseDir/qemu/lib, so it ends up under ~/.config for users, and /etc for priviledged/root. A bad place to store transient runtime data/sockets. You may want to reconsider the paths for pr-helper. I don't intend to change that, so you could take a look at "[libvirt PATCH v2 0/9]".
Re: [PATCH 3/8] qemu-conf: add dbusStateDir
On 2/24/20 4:57 PM, Marc-André Lureau wrote: Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik wrote: On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Location of DBus daemon state configuration, socket, pid... Signed-off-by: Marc-André Lureau --- src/qemu/qemu_conf.c | 4 src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR); +cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); +cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); cfg->configBaseDir = virGetUserConfigDirectory(); Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init. However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to: $cfg->libDir/domain-$shortName/ This way you wouldn't need to care about domain's shortname in the next patch. Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too? I don't know. But since there are some releases which would store data under slirpStateDir I don't think we can change this. On daemon restart (with newer version) the new libvirtd wouldn't see the old dir. Michal
Re: [PATCH 3/8] qemu-conf: add dbusStateDir
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik wrote: > > On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Location of DBus daemon state configuration, socket, pid... > > > > Signed-off-by: Marc-André Lureau > > --- > > src/qemu/qemu_conf.c | 4 > > src/qemu/qemu_conf.h | 1 + > > 2 files changed, 5 insertions(+) > > > > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > > index e1fea390c7..ade12dac6c 100644 > > --- a/src/qemu/qemu_conf.c > > +++ b/src/qemu/qemu_conf.c > > @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool > > privileged) > > > > cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", > > LOCALSTATEDIR); > > > > +cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", > > LOCALSTATEDIR); > > + > > cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", > > LOCALSTATEDIR); > > cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); > > cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); > > @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool > > privileged) > > cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); > > > > cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); > > +cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); > > > > cfg->configBaseDir = virGetUserConfigDirectory(); > > Instead of doing practically the same in two branches, you can achieve > the same result with just one line if you put the call just below > cfg->slirpStateDir init. > > However, do we need this to be in a special directory instead of per VM > private directory? The way I implemented PR helper was that the socket > it creates and to which qemu connects is stored under vm->priv->libDir > which is initialized in qemuDomainSetPrivatePaths() to: > >$cfg->libDir/domain-$shortName/ > > This way you wouldn't need to care about domain's shortname in the next > patch. Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too?
Re: [PATCH 3/8] qemu-conf: add dbusStateDir
On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Location of DBus daemon state configuration, socket, pid... Signed-off-by: Marc-André Lureau --- src/qemu/qemu_conf.c | 4 src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR); +cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); +cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); cfg->configBaseDir = virGetUserConfigDirectory(); Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init. However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to: $cfg->libDir/domain-$shortName/ This way you wouldn't need to care about domain's shortname in the next patch. Michal
[libvirt] [PATCH 3/8] qemu-conf: add dbusStateDir
From: Marc-André Lureau Location of DBus daemon state configuration, socket, pid... Signed-off-by: Marc-André Lureau --- src/qemu/qemu_conf.c | 4 src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR); +cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); +cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); cfg->configBaseDir = virGetUserConfigDirectory(); @@ -274,6 +277,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->stateDir); VIR_FREE(cfg->swtpmStateDir); VIR_FREE(cfg->slirpStateDir); +VIR_FREE(cfg->dbusStateDir); VIR_FREE(cfg->libDir); VIR_FREE(cfg->cacheDir); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 6354925e0b..de6430cf97 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -97,6 +97,7 @@ struct _virQEMUDriverConfig { char *stateDir; char *swtpmStateDir; char *slirpStateDir; +char *dbusStateDir; /* These two directories are ones QEMU processes use (so must match * the QEMU user/group */ char *libDir; -- 2.25.0.rc2.1.g09a9a1a997 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list