Re: [libvirt] [PATCH] qemu: Don't bother user with libvirt-internal paths

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&gt; > >> --- > >> 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