8:51 a.m.
These 2 patches should address your points. I've also used
VIR_APPEND_ELEMENT in another function (1st patch).
At your service should you have any other comment.
Matthieu Coudron
2014-02-05 Michal Privoznik <mprivozn(a)redhat.com>:
On 04.02.2014 10:37, Teto wrote:
>
> Hi,
>
> The following patch was generated with format patch & checked with
> syntax-check. It is really short and adds a new function
> virDomainFSInsert which is called when Qemu driver is requested t
> attach a filesystem device.
>
> Matt
>
>
> 0001-This-commit-allows-to-register-filesystem-in-qemu-vi.patch
>
>
> From f8c0612c48c06c61199693743d98c251ba4d887e Mon Sep 17 00:00:00 2001
> From: Matt<mattator(a)gmail.com>
> Date: Mon, 3 Feb 2014 17:42:56 +0100
> Subject: [PATCH] This commit allows to register <filesystem> in qemu via
> the
> attach_device function (which would previsouly return an error).
>
> For this purpose I've introduced a new function "virDomainFSInsert"
and
> added the necessary code in the qemu driver.
>
> It compares filesystems based on their "destination" folder. So if 2
> filesystems share a same destination, they are considered equal and the
> Qemu driver would reject a new insertion.
> ---
> src/conf/domain_conf.c | 12 ++++++++++++
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 18 ++++++++++++++++++
> 4 files changed, 32 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 28e24f9..3f4dbfe 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17933,6 +17933,18 @@ virDiskNameToBusDeviceIndex(virDomainDiskDefPtr
> disk,
> return 0;
> }
>
> +
> +int
> +virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs)
> +{
> +
> + if (VIR_REALLOC_N(def->fss, def->nfss+1) < 0)
> + return -1;
> +
> + def->fss[def->nfss++] = fs;
> + return 0;
> +}
> +
While this works perfectly, we can save some lines by calling
VIR_APPEND_ELEMENT().
> virDomainFSDefPtr
> virDomainGetRootFilesystem(virDomainDefPtr def)
> {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index d8f2e49..d749e68 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2555,6 +2555,7 @@ int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr
> disk,
> int *devIdx);
>
> virDomainFSDefPtr virDomainGetRootFilesystem(virDomainDefPtr def);
> +int virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs);
> int virDomainFSIndexByName(virDomainDefPtr def, const char *name);
> int virDomainVideoDefaultType(const virDomainDef *def);
> int virDomainVideoDefaultRAM(const virDomainDef *def, int type);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1a8d088..e872960 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -221,6 +221,7 @@ virDomainFeatureStateTypeFromString;
> virDomainFeatureStateTypeToString;
> virDomainFSDefFree;
> virDomainFSIndexByName;
> +virDomainFSInsert;
> virDomainFSTypeFromString;
> virDomainFSTypeToString;
> virDomainFSWrpolicyTypeFromString;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0128356..f2bac0d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6606,6 +6606,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr
> qemuCaps,
> virDomainHostdevDefPtr hostdev;
> virDomainLeaseDefPtr lease;
> virDomainControllerDefPtr controller;
> + virDomainFSDefPtr fs;
>
> switch (dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
> @@ -6687,6 +6688,23 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr
> qemuCaps,
> dev->data.chr = NULL;
> break;
>
> + case VIR_DOMAIN_DEVICE_FS:
> + {
> + fs = dev->data.fs;
> + if (virDomainFSIndexByName(vmdef, fs->dst) >= 0) {
> + VIR_INFO("Identical FS found");
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("Target already exists"));
> + return -1;
> + }
> +
> + if (virDomainFSInsert(vmdef, fs) < 0) {
> + return -1;
> + }
> + dev->data.fs = NULL;
> + }
> + break;
> +
There is no need to enclose the body in { }. Nor for VIR_INFO.
While at this - can you implement the detach counterpart? Again, in config
level is fine for now.
However, I believe our push policy doesn't allow in patches that are missing
real name (first and last one at least). So can you fix that too?
Michal