Re: [libvirt] [PATCH] qemu: Generate agent socket path if missing
On 23.04.2013 17:40, Jiri Denemark wrote:
On Tue, Apr 09, 2013 at 19:05:28 +0200, Michal Privoznik wrote:
> It's not desired to force users imagine path for a socket they
> are not even supposed to connect to. On the other hand, we
> already have a release where the qemu agent socket path is
> exposed to XML, so we cannot silently drop it from there.
> The new path is generated in form:
>
> $LOCALSTATEDIR/lib/libvirt/qemu/channel/target/$domain.$name
> ---
> libvirt.spec.in | 1 +
> src/Makefile.am | 1 +
> src/conf/domain_conf.c | 34 ++++++++++++++++------------------
> src/qemu/qemu_domain.c | 16 ++++++++++++++++
> 4 files changed, 34 insertions(+), 18 deletions(-)
...
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3d6eef4..967890f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -738,6 +738,22 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X))
> dev->data.chr->targetType =
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
>
> + /* auto generate unix socket path */
> + if (dev->type == VIR_DOMAIN_DEVICE_CHR &&
1: ^^
> + 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 &&
> + (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver))))) {
I think this should fail if path is NULL and
(cfg || (driver && (cfg = virQEMUDriverGetConfig(driver)))) is not true.
On the other hand, do we actually need to check for this? Aren't both
cfg and driver guaranteed to be non-NULL at this point?
And actually as Eric pointed out, driver can be NULL and so does cfg.
The only way where cfg already is not NULL is for (dev->type ==
VIR_DOMAIN_DEVICE_DISK) in which case the [1] condition is false. So in
fact, there is no way for cfg to be other than NULL, hence I can drop
check for cfg being not NULL:
if (dev->type = VIR_DOMAIN_DEVICE_CHR && ...
(driver && (cfg = virQEMUDriverGetConfig(driver)))) {
> +
> + if (virAsprintf(&dev->data.chr->source.data.nix.path,
> + "%s/channel/target/%s.%s",
> + cfg->libDir, def->name,
> + dev->data.chr->target.name) < 0)
> + goto no_memory;
> + dev->data.chr->source.data.nix.listen = true;
> + }
> +
> ret = 0;
>
> cleanup:
Jirka
Michal