Re: [PATCH v2] guest-agent-socket: don't generate default path to config XML

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Mon, Nov 30, 2015 at 12:37:58PM +0100, Pavel Hrdina wrote:
> While we started using for all unix sockets as default one common
> directory based on a guest name it introduced several issues, for example
> with renaming the guest or cloning it.  In general it's not entirely
> bad, but in this case it would be best to hide the auto-generated socket
> path from user and don't export it in the config XML.
>
> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
>
> Version 2:
>    - removed unnecessary changes
>
> src/qemu/qemu_command.c  | 28 ++++++++++++++++++++++++++++
> src/qemu/qemu_command.h  |  1 +
> src/qemu/qemu_domain.c   | 16 ----------------
> src/qemu/qemu_driver.c   |  3 +++
> src/qemu/qemu_process.c  |  3 +++
> tests/qemuhotplugtest.c  |  5 +++++
> tests/qemuxml2argvtest.c |  5 +++++
> 7 files changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2a9fab5..f1bb621 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1262,6 +1262,34 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> }
>
>
> +int
> +qemuUnixSocketGenerate(virDomainDefPtr def,
> +                       virQEMUDriverConfigPtr cfg)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->nchannels; i++) {
> +        virDomainChrDefPtr channel = def->channels[i];
> +
> +        if (channel->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> +            channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
> +            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)
> +                return -1;
> +
> +            channel->source.data.nix.listen = true;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
> static void
> qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>                                      virDomainDeviceAddressType type)
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index bebdd27..0512a23 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -283,6 +283,7 @@ int qemuAssignDevicePCISlots(virDomainDefPtr def,
>                              virDomainPCIAddressSetPtr addrs);
>
> int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
> +int qemuUnixSocketGenerate(virDomainDefPtr def, virQEMUDriverConfigPtr cfg);
> int qemuDomainNetVLAN(virDomainNetDefPtr def);
> int qemuAssignDeviceNetAlias(virDomainDefPtr def, virDomainNetDefPtr net, int idx);
> int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ed21245..7e05289 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1329,22 +1329,6 @@ 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 */
> -    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;
> -    }
> -

For the sake of upgradibility, here we should disregard any paths that
start with cfg->channelTargetDir, maybe even with cfg->configBaseDir.
Users should not be using paths in libvirt's directories if they need to
access it anyway.  Of course we'll need to document that as well.

I just wonder how come we don't need any tests fixed.  We should've been
testing the auto-generation, I believe.  And even if not, we should test
that we don't generate it into the XML now.

Otherwise looks fine, weak ACK due to tests, but I believe they can be
part of the next patch for the removal of these paths.

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list