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@xxxxxxxxx>
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@xxxxxxxxx>
---
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@xxxxxxxxx>
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@xxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list