[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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 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
From 9235e4874266644af8180512e9920c35cfb0f09a Mon Sep 17 00:00:00 2001
From: Matthieu Coudron <mattator 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 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);
 }
 
 virDomainHostdevDefPtr
-- 
1.8.3.2

From ed71d16b697c5a17946e5bef9fb74e6ac5a52fb0 Mon Sep 17 00:00:00 2001
From: Matthieu Coudron <mattator 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 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;
+}
+
 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;
+        }
+        dev->data.fs = NULL;
+        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;
+        break;
+
     default:
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
                        _("persistent detach of device '%s' is not supported"),
-- 
1.8.3.2


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]