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

2014-02-07 Thread Teto 
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 mpriv...@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 Coudronmatta...@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 Coudronmatta...@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 Coudronmatta...@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 Coudronmatta...@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
 +++

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

2014-02-06 Thread Teto 
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 mpriv...@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: Mattmatta...@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 matta...@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 matta...@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
---

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

2014-02-06 Thread Michal Privoznik 

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 Coudronmatta...@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 Coudronmatta...@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 Coudronmatta...@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 Coudronmatta...@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 @@

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

2014-02-05 Thread Michal Privoznik 

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: Mattmatta...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list