5:51 p.m.
I had run the 2 first checks but not the valgrind check.Sorry for the
memleak, hopefully you catched it. Thanks for your help through the
whole process.
Matt
2014-02-06 Michal Privoznik <mprivozn(a)redhat.com>:
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(a)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(a)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(a)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(a)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