10:50 a.m.
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(a)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
2:40 a.m.
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(a)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
3:18 a.m.
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(a)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
3:47 a.m.
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(a)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
4:30 a.m.
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(a)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
3242
days inactive
3243
days old
4 comments
2 participants
participants (2)
-
Martin Kletzander -
Pavel Hrdina