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

[Martin Kletzander]

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 at 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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160107/e5af500d/attachment-0001.sig>