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@xxxxxxxxxx> >> --- >> 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list