[libvirt] [PATCH] Add <filesystem> support to Qemu's attach_device

Michal Privoznik mprivozn at redhat.com
Thu Feb 6 16:28:27 UTC 2014


On 06.02.2014 15:51, Teto wrote:
> 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
>
<snip/>
> 0001-Replaced-VIR_REALLOC_N-by-VIR_APPEND_ELEMENT-in-virD.patch
>
>
>  From 9235e4874266644af8180512e9920c35cfb0f09a Mon Sep 17 00:00:00 2001
> From: Matthieu Coudron<mattator at gmail.com>
> Date: Thu, 6 Feb 2014 15:29:08 +0100
> Subject: [PATCH 1/2] Replaced VIR_REALLOC_N by VIR_APPEND_ELEMENT in
>   virDomainHostdevInsert so that the code gets shorter and more readable
>
> Signed-off-by: Matthieu Coudron<mattator at gmail.com>
> ---
>   src/conf/domain_conf.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 28e24f9..3f3822e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9868,10 +9868,8 @@ virDomainChrTargetTypeToString(int deviceType,
>   int
>   virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr hostdev)
>   {
> -    if (VIR_REALLOC_N(def->hostdevs, def->nhostdevs + 1) < 0)
> -        return -1;
> -    def->hostdevs[def->nhostdevs++]  = hostdev;
> -    return 0;
> +
> +    return VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev);
>   }
>

virDomainHostdevRemove could be changed as well :) I've taken a chance 
and did it.

>   virDomainHostdevDefPtr
> -- 1.8.3.2
>
>
> 0002-filesystem-support-in-device-addition-removal-for-th.patch
>
>
>  From ed71d16b697c5a17946e5bef9fb74e6ac5a52fb0 Mon Sep 17 00:00:00 2001
> From: Matthieu Coudron<mattator at gmail.com>
> Date: Thu, 6 Feb 2014 15:30:07 +0100
> Subject: [PATCH 2/2] <filesystem> support in device addition/removal for the
>   Qemu driver
>
> This commit allows to attach/detach a <filesystem> device in qemu (which would previously return an error).
>
> For this purpose I've introduced 2 new functions "virDomainFSInsert" and "virDomainFSRemove" 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.
>
> Signed-off-by: Matthieu Coudron<mattator at gmail.com>
> ---
>   src/conf/domain_conf.c   | 30 ++++++++++++++++++++++++++++++
>   src/conf/domain_conf.h   |  3 +++
>   src/libvirt_private.syms |  2 ++
>   src/qemu/qemu_driver.c   | 30 ++++++++++++++++++++++++++++++
>   4 files changed, 65 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3f3822e..9e75b21 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17931,6 +17931,36 @@ virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
>       return 0;
>   }
>
> +
> +int
> +virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs)
> +{
> +
> +    return VIR_APPEND_ELEMENT(def->fss, def->nfss, fs);
> +}
> +
> +virDomainFSDefPtr
> +virDomainFSRemove(virDomainDefPtr def, size_t i)
> +{
> +    virDomainFSDefPtr fs = def->fss[i];
> +
> +    if (def->nfss > 1) {
> +
> +        memmove(def->fss + i,
> +                def->fss + i + 1,
> +                sizeof(*def->fss) *
> +                (def->nfss - (i + 1)));
> +        def->nfss--;
> +        if (VIR_REALLOC_N(def->fss, def->nfss) < 0) {
> +            /* ignore, harmless */
> +        }
> +    } else {
> +        VIR_FREE(def->fss);
> +        def->nfss = 0;
> +    }
> +    return fs;
> +}
> +

Again, we have VIR_DELETE_ELEMENT, but I've changed this too.

>   virDomainFSDefPtr
>   virDomainGetRootFilesystem(virDomainDefPtr def)
>   {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index d8f2e49..9acb105 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2555,7 +2555,10 @@ int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk,
>                                   int *devIdx);
>
>   virDomainFSDefPtr virDomainGetRootFilesystem(virDomainDefPtr def);
> +int virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs);
>   int virDomainFSIndexByName(virDomainDefPtr def, const char *name);
> +virDomainFSDefPtr virDomainFSRemove(virDomainDefPtr def, size_t i);
> +
>   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 c5a7637..2c9536a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -221,6 +221,8 @@ virDomainFeatureStateTypeFromString;
>   virDomainFeatureStateTypeToString;
>   virDomainFSDefFree;
>   virDomainFSIndexByName;
> +virDomainFSInsert;
> +virDomainFSRemove;
>   virDomainFSTypeFromString;
>   virDomainFSTypeToString;
>   virDomainFSWrpolicyTypeFromString;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 38a48db..8d7b228 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,20 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
>           dev->data.chr = NULL;
>           break;
>
> +    case VIR_DOMAIN_DEVICE_FS:
> +        fs = dev->data.fs;
> +        if (virDomainFSIndexByName(vmdef, fs->dst) >= 0) {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                         "%s", _("Target already exists"));
> +            return -1;
> +        }
> +
> +        if (virDomainFSInsert(vmdef, fs) < 0) {
> +            return -1;
> +        }

We don't tend to enclose a single line in brackets.

> +        dev->data.fs = NULL;

While this is okay, because virDomainFSInsert steals the object ...

> +        break;
> +
>       default:
>            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                           _("persistent attach of device '%s' is not supported"),
> @@ -6707,6 +6722,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>       virDomainLeaseDefPtr lease, det_lease;
>       virDomainControllerDefPtr cont, det_cont;
>       virDomainChrDefPtr chr;
> +    virDomainFSDefPtr fs;
>       int idx;
>       char mac[VIR_MAC_STRING_BUFLEN];
>
> @@ -6783,6 +6799,20 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>           dev->data.chr = NULL;
>           break;
>
> +    case VIR_DOMAIN_DEVICE_FS:
> +        fs = dev->data.fs;
> +        idx = virDomainFSIndexByName(vmdef, fs->dst);
> +        if (idx < 0) {
> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                           _("no matching filesystem device was found"));
> +            return -1;
> +        }
> +
> +        fs = virDomainFSRemove(vmdef, idx);
> +        virDomainFSDefFree(fs);
> +        dev->data.fs = NULL;

... setting to NULL here, will lead to a memleak.

> +        break;
> +
>       default:
>           virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                          _("persistent detach of device '%s' is not supported"),
> -- 1.8.3.2
>

Despite small nits, I'm givin ACK, fixing the issues and pushing. 
Congratulations on your first libvirt patches!

Michal




More information about the libvir-list mailing list