[libvirt] [PATCH] qemu: Don't bother user with libvirt-internal paths
If user defines a virtio channel with UNIX socket backend and doesn't care about the path for the socket (e.g. qemu-agent channel), we still generate it into the persistent XML. Moreover when then user renames the domain, due to its persistent socket path saved into the per-domain directory, it will not start. So let's forget about old generated paths and also stop putting them into the persistent definition. https://bugzilla.redhat.com/show_bug.cgi?id=1278068 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++++++ src/qemu/qemu_domain.c | 21 +++++++++++---------- .../qemuxml2argv-channel-virtio-unix.args | 5 ++++- .../qemuxml2argv-channel-virtio-unix.xml | 4 ++++ 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 66ca11152ad8..c5127cfa04f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10510,6 +10510,18 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } + if (channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/domain-%s/%s", + cfg->channelTargetDir, def->name, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + goto error; + + channel->source.data.nix.listen = true; + } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC) && channel->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { /* spicevmc was originally introduced via a -device diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bb8d47f9293d..73fc79dab56b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1329,20 +1329,21 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, ARCH_IS_S390(def->os.arch)) dev->data.controller->model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI; - /* auto generate unix socket path */ + /* clear auto generated unix socket path */ if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && dev->data.chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && dev->data.chr->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && - !dev->data.chr->source.data.nix.path) { - if (virAsprintf(&dev->data.chr->source.data.nix.path, - "%s/domain-%s/%s", - cfg->channelTargetDir, def->name, - dev->data.chr->target.name ? dev->data.chr->target.name - : "unknown.sock") < 0) - goto cleanup; - - dev->data.chr->source.data.nix.listen = true; + dev->data.chr->source.data.nix.path && + STRPREFIX(dev->data.chr->source.data.nix.path, cfg->channelTargetDir)) { + /* + * If the address is generated by us (starts with our + * channel dir), we should not keep it in the persistent + * XML. If libvirt is the one who generated it, users + * shouldn't care about that. If they do, they are + * supposed to set it themselves. + */ + VIR_FREE(dev->data.chr->source.data.nix.path); } /* forbid capabilities mode hostdev in this kind of hypervisor */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args index 94375b85df69..6a0b1f373603 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.args @@ -33,4 +33,7 @@ nowait \ id=channel1 \ -chardev socket,id=charchannel2,path=/tmp/domain-QEMUGuest1/ble,server,nowait \ -device virtserialport,bus=virtio-serial0.0,nr=3,chardev=charchannel2,\ -id=channel2,name=ble +id=channel2,name=ble \ +-chardev socket,id=charchannel3,path=/tmp/domain-QEMUGuest1/fdsa,server,nowait \ +-device virtserialport,bus=virtio-serial0.0,nr=4,chardev=charchannel3,\ +id=channel3,name=fdsa diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.xml b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.xml index 7fac943389bf..405dff8389bf 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-unix.xml @@ -32,6 +32,10 @@ <channel type='unix'> <target type='virtio' name='ble'/> </channel> + <channel type='unix'> + <source path='/tmp/domain-oldname/fdsa'/> + <target type='virtio' name='fdsa'/> + </channel> <memballoon model='none'/> </devices> </domain> -- 2.7.0
On Wed, Jan 06, 2016 at 05:50:57PM +0100, Martin Kletzander wrote:
If user defines a virtio channel with UNIX socket backend and doesn't care about the path for the socket (e.g. qemu-agent channel), we still generate it into the persistent XML. Moreover when then user renames the domain, due to its persistent socket path saved into the per-domain directory, it will not start. So let's forget about old generated paths and also stop putting them into the persistent definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1278068
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++++++ src/qemu/qemu_domain.c | 21 +++++++++++---------- .../qemuxml2argv-channel-virtio-unix.args | 5 ++++- .../qemuxml2argv-channel-virtio-unix.xml | 4 ++++ 4 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 66ca11152ad8..c5127cfa04f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10510,6 +10510,18 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/domain-%s/%s", + cfg->channelTargetDir, def->name, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + goto error; + + channel->source.data.nix.listen = true; + } +
I don't like this. The qemuBuildCommandLine function should only create a command line, not modify the domain definition. I know, there are other places, that do the same, but let's try to avoid it.
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC) && channel->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { /* spicevmc was originally introduced via a -device diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
[...] Otherwise it looks good. Pavel
On Thu, Jan 07, 2016 at 09:40:29AM +0100, Pavel Hrdina wrote:
On Wed, Jan 06, 2016 at 05:50:57PM +0100, Martin Kletzander wrote:
If user defines a virtio channel with UNIX socket backend and doesn't care about the path for the socket (e.g. qemu-agent channel), we still generate it into the persistent XML. Moreover when then user renames the domain, due to its persistent socket path saved into the per-domain directory, it will not start. So let's forget about old generated paths and also stop putting them into the persistent definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1278068
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++++++ src/qemu/qemu_domain.c | 21 +++++++++++---------- .../qemuxml2argv-channel-virtio-unix.args | 5 ++++- .../qemuxml2argv-channel-virtio-unix.xml | 4 ++++ 4 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 66ca11152ad8..c5127cfa04f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10510,6 +10510,18 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/domain-%s/%s", + cfg->channelTargetDir, def->name, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + goto error; + + channel->source.data.nix.listen = true; + } +
I don't like this. The qemuBuildCommandLine function should only create a command line, not modify the domain definition. I know, there are other places, that do the same, but let's try to avoid it.
But it's the only function that gets involved from the tests. We should have different places for generating stuff and formatting command line, I know, it looks awful. But for now it fixes the issue and I wasn't sure whether Jiri wasn't messing with any other qemuProcess{Init,Launch,Start,Whatever} renames and rebuilds. Is there any place now that we could use without refactoring the whole thing?
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC) && channel->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { /* spicevmc was originally introduced via a -device diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
[...]
Otherwise it looks good.
Pavel
On Thu, Jan 07, 2016 at 10:18:26AM +0100, Martin Kletzander wrote:
On Thu, Jan 07, 2016 at 09:40:29AM +0100, Pavel Hrdina wrote:
On Wed, Jan 06, 2016 at 05:50:57PM +0100, Martin Kletzander wrote:
If user defines a virtio channel with UNIX socket backend and doesn't care about the path for the socket (e.g. qemu-agent channel), we still generate it into the persistent XML. Moreover when then user renames the domain, due to its persistent socket path saved into the per-domain directory, it will not start. So let's forget about old generated paths and also stop putting them into the persistent definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1278068
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++++++ src/qemu/qemu_domain.c | 21 +++++++++++---------- .../qemuxml2argv-channel-virtio-unix.args | 5 ++++- .../qemuxml2argv-channel-virtio-unix.xml | 4 ++++ 4 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 66ca11152ad8..c5127cfa04f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10510,6 +10510,18 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/domain-%s/%s", + cfg->channelTargetDir, def->name, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + goto error; + + channel->source.data.nix.listen = true; + } +
I don't like this. The qemuBuildCommandLine function should only create a command line, not modify the domain definition. I know, there are other places, that do the same, but let's try to avoid it.
But it's the only function that gets involved from the tests. We should have different places for generating stuff and formatting command line, I know, it looks awful. But for now it fixes the issue and I wasn't sure whether Jiri wasn't messing with any other qemuProcess{Init,Launch,Start,Whatever} renames and rebuilds. Is there any place now that we could use without refactoring the whole thing?
Without refactoring, probably not. Jiri finished is work with qemuProcess* functions. I'm not saying to refactor it in scope of this patch, I just wanted to take a minute and point this out. You can use the similar way like in ma patch for this issue or just add a TODO comment and refactor it later.
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC) && channel->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { /* spicevmc was originally introduced via a -device diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
[...]
Otherwise it looks good.
Pavel
On Thu, Jan 07, 2016 at 10:47:46AM +0100, Pavel Hrdina wrote:
On Thu, Jan 07, 2016 at 10:18:26AM +0100, Martin Kletzander wrote:
On Thu, Jan 07, 2016 at 09:40:29AM +0100, Pavel Hrdina wrote:
On Wed, Jan 06, 2016 at 05:50:57PM +0100, Martin Kletzander wrote:
If user defines a virtio channel with UNIX socket backend and doesn't care about the path for the socket (e.g. qemu-agent channel), we still generate it into the persistent XML. Moreover when then user renames the domain, due to its persistent socket path saved into the per-domain directory, it will not start. So let's forget about old generated paths and also stop putting them into the persistent definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1278068
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++++++ src/qemu/qemu_domain.c | 21 +++++++++++---------- .../qemuxml2argv-channel-virtio-unix.args | 5 ++++- .../qemuxml2argv-channel-virtio-unix.xml | 4 ++++ 4 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 66ca11152ad8..c5127cfa04f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10510,6 +10510,18 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
+ if (channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/domain-%s/%s", + cfg->channelTargetDir, def->name, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + goto error; + + channel->source.data.nix.listen = true; + } +
I don't like this. The qemuBuildCommandLine function should only create a command line, not modify the domain definition. I know, there are other places, that do the same, but let's try to avoid it.
But it's the only function that gets involved from the tests. We should have different places for generating stuff and formatting command line, I know, it looks awful. But for now it fixes the issue and I wasn't sure whether Jiri wasn't messing with any other qemuProcess{Init,Launch,Start,Whatever} renames and rebuilds. Is there any place now that we could use without refactoring the whole thing?
Without refactoring, probably not. Jiri finished is work with qemuProcess* functions. I'm not saying to refactor it in scope of this patch, I just wanted to take a minute and point this out. You can use the similar way like in ma patch for this issue or just add a TODO comment and refactor it later.
OK, I added the TODO: and pushed this. You are right, we should do something about this, but this is not the right place to do it. Moreover, there are other things as well that should be part of the code movement, so it needs more decisions from other people as well. Thanks for the review.
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC) && channel->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) { /* spicevmc was originally introduced via a -device diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
[...]
Otherwise it looks good.
Pavel
participants (2)
-
Martin Kletzander -
Pavel Hrdina