[libvirt] [PATCH] nodedev: Return the parent for a virNodeDevicePtr struct

John Ferlan posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1486591432-12613-1-git-send-email-jferlan@redhat.com
src/conf/node_device_conf.c          |  4 ++--
src/node_device/node_device_driver.c | 10 ++++++++--
src/test/test_driver.c               |  6 +++++-
3 files changed, 15 insertions(+), 5 deletions(-)
[libvirt] [PATCH] nodedev: Return the parent for a virNodeDevicePtr struct
Posted by John Ferlan 7 years, 1 month ago
When the 'parent' was added to the virNodeDevicePtr structure
by commit id 'e8a4ea75a' the 'parent' field was not properly filled
in when a virGetNodeDevice call was made within driver/config code.
Only the device name was ever filled in. Fetching the parent required
a second trip via virNodeDeviceGetParent into the node device lookup
code was required in order to retrieve the specific parent field (and
still the parent field was never filled in although it was free'd).

Since we have the data when we initially call virGetNodeDevice from
within driver/node_config code - let's just fill in the parent field
as well for anyone that wants it without requiring another trip into
the node_device lookup just to get the parent.

This will allow API's such as virConnectListAllNodeDevices,
virNodeDeviceLookupByName, and virNodeDeviceLookupSCSIHostByWWN
to retrieve both name and parent in the returned virNodeDevicePtr.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---

 Found this while working on some vHBA in the domain code when I was
 thinking about using a virConnectListAllNodeDevices... The returned
 structures didn't have the ->parent filled in... 

 src/conf/node_device_conf.c          |  4 ++--
 src/node_device/node_device_driver.c | 10 ++++++++--
 src/test/test_driver.c               |  6 +++++-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 4d3268f..f6d7692 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2158,8 +2158,8 @@ virNodeDeviceObjListExport(virConnectPtr conn,
         if ((!filter || filter(conn, devobj->def)) &&
             virNodeDeviceMatch(devobj, flags)) {
             if (devices) {
-                if (!(device = virGetNodeDevice(conn,
-                                                devobj->def->name))) {
+                if (!(device = virGetNodeDevice(conn, devobj->def->name)) ||
+                    VIR_STRDUP(device->parent, devobj->def->parent) < 0) {
                     virNodeDeviceObjUnlock(devobj);
                     goto cleanup;
                 }
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 5238e23..4900e32 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -261,7 +261,10 @@ nodeDeviceLookupByName(virConnectPtr conn, const char *name)
     if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0)
         goto cleanup;
 
-    ret = virGetNodeDevice(conn, name);
+    if ((ret = virGetNodeDevice(conn, name))) {
+        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
+            virObjectUnref(ret);
+    }
 
  cleanup:
     if (obj)
@@ -302,7 +305,10 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
                         if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, obj->def) < 0)
                             goto out;
 
-                        dev = virGetNodeDevice(conn, obj->def->name);
+                        if ((dev = virGetNodeDevice(conn, obj->def->name))) {
+                            if (VIR_STRDUP(dev->parent, obj->def->parent) < 0)
+                                virObjectUnref(dev);
+                        }
                         virNodeDeviceObjUnlock(obj);
                         goto out;
                     }
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6820298..8dd738b 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5450,7 +5450,10 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
         goto cleanup;
     }
 
-    ret = virGetNodeDevice(conn, name);
+    if ((ret = virGetNodeDevice(conn, name))) {
+        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
+            virObjectUnref(ret);
+    }
 
  cleanup:
     if (obj)
@@ -5648,6 +5651,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
                                            0);
 
     dev = virGetNodeDevice(conn, def->name);
+    ignore_value(VIR_STRDUP(def->parent, def->parent));
     def = NULL;
  cleanup:
     testDriverUnlock(driver);
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Return the parent for a virNodeDevicePtr struct
Posted by Bjoern Walk 7 years, 1 month ago
John Ferlan <jferlan@redhat.com> [2017-02-08, 11:06PM +0100]:
>When the 'parent' was added to the virNodeDevicePtr structure
>by commit id 'e8a4ea75a' the 'parent' field was not properly filled
>in when a virGetNodeDevice call was made within driver/config code.
>Only the device name was ever filled in. Fetching the parent required
>a second trip via virNodeDeviceGetParent into the node device lookup
>code was required in order to retrieve the specific parent field (and
>still the parent field was never filled in although it was free'd).
>
>Since we have the data when we initially call virGetNodeDevice from
>within driver/node_config code - let's just fill in the parent field
>as well for anyone that wants it without requiring another trip into
>the node_device lookup just to get the parent.
>
>This will allow API's such as virConnectListAllNodeDevices,
>virNodeDeviceLookupByName, and virNodeDeviceLookupSCSIHostByWWN
>to retrieve both name and parent in the returned virNodeDevicePtr.
>
>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
>
> Found this while working on some vHBA in the domain code when I was
> thinking about using a virConnectListAllNodeDevices... The returned
> structures didn't have the ->parent filled in...
>
> src/conf/node_device_conf.c          |  4 ++--
> src/node_device/node_device_driver.c | 10 ++++++++--
> src/test/test_driver.c               |  6 +++++-
> 3 files changed, 15 insertions(+), 5 deletions(-)
>
>diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>index 4d3268f..f6d7692 100644
>--- a/src/conf/node_device_conf.c
>+++ b/src/conf/node_device_conf.c
>@@ -2158,8 +2158,8 @@ virNodeDeviceObjListExport(virConnectPtr conn,
>         if ((!filter || filter(conn, devobj->def)) &&
>             virNodeDeviceMatch(devobj, flags)) {
>             if (devices) {
>-                if (!(device = virGetNodeDevice(conn,
>-                                                devobj->def->name))) {
>+                if (!(device = virGetNodeDevice(conn, devobj->def->name)) ||
>+                    VIR_STRDUP(device->parent, devobj->def->parent) < 0) {
>                     virNodeDeviceObjUnlock(devobj);
>                     goto cleanup;
>                 }
>diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
>index 5238e23..4900e32 100644
>--- a/src/node_device/node_device_driver.c
>+++ b/src/node_device/node_device_driver.c
>@@ -261,7 +261,10 @@ nodeDeviceLookupByName(virConnectPtr conn, const char *name)
>     if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0)
>         goto cleanup;
>
>-    ret = virGetNodeDevice(conn, name);
>+    if ((ret = virGetNodeDevice(conn, name))) {
>+        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
>+            virObjectUnref(ret);
>+    }
>
>  cleanup:
>     if (obj)
>@@ -302,7 +305,10 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>                         if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, obj->def) < 0)
>                             goto out;
>
>-                        dev = virGetNodeDevice(conn, obj->def->name);
>+                        if ((dev = virGetNodeDevice(conn, obj->def->name))) {
>+                            if (VIR_STRDUP(dev->parent, obj->def->parent) < 0)
>+                                virObjectUnref(dev);
>+                        }
>                         virNodeDeviceObjUnlock(obj);
>                         goto out;
>                     }
>diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>index 6820298..8dd738b 100644
>--- a/src/test/test_driver.c
>+++ b/src/test/test_driver.c
>@@ -5450,7 +5450,10 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
>         goto cleanup;
>     }
>
>-    ret = virGetNodeDevice(conn, name);
>+    if ((ret = virGetNodeDevice(conn, name))) {
>+        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
>+            virObjectUnref(ret);
>+    }
>
>  cleanup:
>     if (obj)
>@@ -5648,6 +5651,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
>                                            0);
>
>     dev = virGetNodeDevice(conn, def->name);
>+    ignore_value(VIR_STRDUP(def->parent, def->parent));

One of those should be dev probably.

>     def = NULL;
>  cleanup:
>     testDriverUnlock(driver);
>-- 
>2.7.4
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
>

-- 
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Return the parent for a virNodeDevicePtr struct
Posted by John Ferlan 7 years, 1 month ago

On 02/09/2017 07:39 AM, Bjoern Walk wrote:

[...]

>>                     }
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 6820298..8dd738b 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -5450,7 +5450,10 @@ testNodeDeviceLookupByName(virConnectPtr conn,
>> const char *name)
>>         goto cleanup;
>>     }
>>
>> -    ret = virGetNodeDevice(conn, name);
>> +    if ((ret = virGetNodeDevice(conn, name))) {
>> +        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
>> +            virObjectUnref(ret);
>> +    }
>>
>>  cleanup:
>>     if (obj)
>> @@ -5648,6 +5651,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
>>                                            0);
>>
>>     dev = virGetNodeDevice(conn, def->name);
>> +    ignore_value(VIR_STRDUP(def->parent, def->parent));
> 
> One of those should be dev probably.
> 

Oh right goog catch... It should be VIR_STRDUP(dev->parent, def->parent)

I've fixed in my local branch

John
>>     def = NULL;
>>  cleanup:
>>     testDriverUnlock(driver);
>> -- 
>> 2.7.4
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Return the parent for a virNodeDevicePtr struct
Posted by Ján Tomko 7 years, 1 month ago
On Thu, Feb 09, 2017 at 02:23:47PM -0500, John Ferlan wrote:
>
>
>On 02/09/2017 07:39 AM, Bjoern Walk wrote:
>
>[...]
>
>>>                     }
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index 6820298..8dd738b 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -5450,7 +5450,10 @@ testNodeDeviceLookupByName(virConnectPtr conn,
>>> const char *name)
>>>         goto cleanup;
>>>     }
>>>
>>> -    ret = virGetNodeDevice(conn, name);
>>> +    if ((ret = virGetNodeDevice(conn, name))) {
>>> +        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
>>> +            virObjectUnref(ret);
>>> +    }
>>>
>>>  cleanup:
>>>     if (obj)
>>> @@ -5648,6 +5651,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
>>>                                            0);
>>>
>>>     dev = virGetNodeDevice(conn, def->name);
>>> +    ignore_value(VIR_STRDUP(def->parent, def->parent));
>>
>> One of those should be dev probably.
>>
>
>Oh right goog catch... It should be VIR_STRDUP(dev->parent, def->parent)
>
>I've fixed in my local branch

ACK with that fix.

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Return the parent for a virNodeDevicePtr struct
Posted by John Ferlan 7 years, 1 month ago
ping - no ACK on the only comment...

Tks -

John

On 02/08/2017 05:03 PM, John Ferlan wrote:
> When the 'parent' was added to the virNodeDevicePtr structure
> by commit id 'e8a4ea75a' the 'parent' field was not properly filled
> in when a virGetNodeDevice call was made within driver/config code.
> Only the device name was ever filled in. Fetching the parent required
> a second trip via virNodeDeviceGetParent into the node device lookup
> code was required in order to retrieve the specific parent field (and
> still the parent field was never filled in although it was free'd).
> 
> Since we have the data when we initially call virGetNodeDevice from
> within driver/node_config code - let's just fill in the parent field
> as well for anyone that wants it without requiring another trip into
> the node_device lookup just to get the parent.
> 
> This will allow API's such as virConnectListAllNodeDevices,
> virNodeDeviceLookupByName, and virNodeDeviceLookupSCSIHostByWWN
> to retrieve both name and parent in the returned virNodeDevicePtr.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> 
>  Found this while working on some vHBA in the domain code when I was
>  thinking about using a virConnectListAllNodeDevices... The returned
>  structures didn't have the ->parent filled in... 
> 
>  src/conf/node_device_conf.c          |  4 ++--
>  src/node_device/node_device_driver.c | 10 ++++++++--
>  src/test/test_driver.c               |  6 +++++-
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 4d3268f..f6d7692 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2158,8 +2158,8 @@ virNodeDeviceObjListExport(virConnectPtr conn,
>          if ((!filter || filter(conn, devobj->def)) &&
>              virNodeDeviceMatch(devobj, flags)) {
>              if (devices) {
> -                if (!(device = virGetNodeDevice(conn,
> -                                                devobj->def->name))) {
> +                if (!(device = virGetNodeDevice(conn, devobj->def->name)) ||
> +                    VIR_STRDUP(device->parent, devobj->def->parent) < 0) {
>                      virNodeDeviceObjUnlock(devobj);
>                      goto cleanup;
>                  }
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 5238e23..4900e32 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -261,7 +261,10 @@ nodeDeviceLookupByName(virConnectPtr conn, const char *name)
>      if (virNodeDeviceLookupByNameEnsureACL(conn, obj->def) < 0)
>          goto cleanup;
>  
> -    ret = virGetNodeDevice(conn, name);
> +    if ((ret = virGetNodeDevice(conn, name))) {
> +        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
> +            virObjectUnref(ret);
> +    }
>  
>   cleanup:
>      if (obj)
> @@ -302,7 +305,10 @@ nodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>                          if (virNodeDeviceLookupSCSIHostByWWNEnsureACL(conn, obj->def) < 0)
>                              goto out;
>  
> -                        dev = virGetNodeDevice(conn, obj->def->name);
> +                        if ((dev = virGetNodeDevice(conn, obj->def->name))) {
> +                            if (VIR_STRDUP(dev->parent, obj->def->parent) < 0)
> +                                virObjectUnref(dev);
> +                        }
>                          virNodeDeviceObjUnlock(obj);
>                          goto out;
>                      }
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 6820298..8dd738b 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -5450,7 +5450,10 @@ testNodeDeviceLookupByName(virConnectPtr conn, const char *name)
>          goto cleanup;
>      }
>  
> -    ret = virGetNodeDevice(conn, name);
> +    if ((ret = virGetNodeDevice(conn, name))) {
> +        if (VIR_STRDUP(ret->parent, obj->def->parent) < 0)
> +            virObjectUnref(ret);
> +    }
>  
>   cleanup:
>      if (obj)
> @@ -5648,6 +5651,7 @@ testNodeDeviceCreateXML(virConnectPtr conn,
>                                             0);
>  
>      dev = virGetNodeDevice(conn, def->name);
> +    ignore_value(VIR_STRDUP(def->parent, def->parent));
>      def = NULL;
>   cleanup:
>      testDriverUnlock(driver);
> 

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