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&lt;mattator(a)gmail.com&gt; > 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

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&lt;mattator(a)gmail.com&gt; > 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&lt;mattator(a)gmail.com&gt; > --- > 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&lt;mattator(a)gmail.com&gt; > 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&lt;mattator(a)gmail.com&gt; > --- > 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