[PATCH 05/11] tools: add option inactive to nodedev-dumpxml

Boris Fiuczynski posted 11 patches 7 months, 3 weeks ago
There is a newer version of this series
[PATCH 05/11] tools: add option inactive to nodedev-dumpxml
Posted by Boris Fiuczynski 7 months, 3 weeks ago
Allow to dump the XML of the persisted mdev when the mdev has been
started instead of the current state only.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
 docs/manpages/virsh.rst |  7 +++++--
 tools/virsh-nodedev.c   | 15 ++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index ed1027e133..3a814dccd2 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -5422,14 +5422,17 @@ nodedev-dumpxml
 
 ::
 
-   nodedev-dumpxml [--xpath EXPRESSION] [--wrap] device
+   nodedev-dumpxml [--inactive] [--xpath EXPRESSION] [--wrap] device
 
 Dump a <device> XML representation for the given node device, including
 such information as the device name, which bus owns the device, the
 vendor and product id, and any capabilities of the device usable by
 libvirt (such as whether device reset is supported). *device* can
 be either device name or wwn pair in "wwnn,wwpn" format (only works
-for HBA).
+for HBA). An additional option affecting the XML dump may be
+used. *--inactive* tells virsh to dump the node device configuration
+that will be used on next start of the node device as opposed to the
+current node device configuration.
 
 If the **--xpath** argument provides an XPath expression, it will be
 evaluated against the output XML and only those matching nodes will
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index fb38fd7fcc..54e192d619 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -575,6 +575,10 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = {
      .help = N_("device name or wwn pair in 'wwnn,wwpn' format"),
      .completer = virshNodeDeviceNameCompleter,
     },
+    {.name = "inactive",
+     .type = VSH_OT_BOOL,
+     .help = N_("show inactive defined XML"),
+    },
     {.name = "xpath",
      .type = VSH_OT_STRING,
      .flags = VSH_OFLAG_REQ_OPT,
@@ -594,6 +598,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
     g_autoptr(virshNodeDevice) device = NULL;
     g_autofree char *xml = NULL;
     const char *device_value = NULL;
+    unsigned int flags = 0;
     bool wrap = vshCommandOptBool(cmd, "wrap");
     const char *xpath = NULL;
 
@@ -608,7 +613,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
     if (!device)
         return false;
 
-    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
+    if (vshCommandOptBool(cmd, "inactive")) {
+        flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
+        if (!virNodeDeviceIsPersistent(device)) {
+            vshError(ctl, _("Node device '%1$s' is not persistent"), device_value);
+            return false;
+        }
+    }
+
+    if (!(xml = virNodeDeviceGetXMLDesc(device, flags)))
         return false;
 
     return virshDumpXML(ctl, xml, "node-device", xpath, wrap);
-- 
2.42.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 05/11] tools: add option inactive to nodedev-dumpxml
Posted by Jonathon Jongsma 7 months, 1 week ago
On 1/19/24 10:38 AM, Boris Fiuczynski wrote:
> Allow to dump the XML of the persisted mdev when the mdev has been
> started instead of the current state only.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
>   docs/manpages/virsh.rst |  7 +++++--
>   tools/virsh-nodedev.c   | 15 ++++++++++++++-
>   2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index ed1027e133..3a814dccd2 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -5422,14 +5422,17 @@ nodedev-dumpxml
>   
>   ::
>   
> -   nodedev-dumpxml [--xpath EXPRESSION] [--wrap] device
> +   nodedev-dumpxml [--inactive] [--xpath EXPRESSION] [--wrap] device
>   
>   Dump a <device> XML representation for the given node device, including
>   such information as the device name, which bus owns the device, the
>   vendor and product id, and any capabilities of the device usable by
>   libvirt (such as whether device reset is supported). *device* can
>   be either device name or wwn pair in "wwnn,wwpn" format (only works
> -for HBA).
> +for HBA). An additional option affecting the XML dump may be
> +used. *--inactive* tells virsh to dump the node device configuration
> +that will be used on next start of the node device as opposed to the
> +current node device configuration.
>   
>   If the **--xpath** argument provides an XPath expression, it will be
>   evaluated against the output XML and only those matching nodes will
> diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
> index fb38fd7fcc..54e192d619 100644
> --- a/tools/virsh-nodedev.c
> +++ b/tools/virsh-nodedev.c
> @@ -575,6 +575,10 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = {
>        .help = N_("device name or wwn pair in 'wwnn,wwpn' format"),
>        .completer = virshNodeDeviceNameCompleter,
>       },
> +    {.name = "inactive",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("show inactive defined XML"),
> +    },
>       {.name = "xpath",
>        .type = VSH_OT_STRING,
>        .flags = VSH_OFLAG_REQ_OPT,
> @@ -594,6 +598,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
>       g_autoptr(virshNodeDevice) device = NULL;
>       g_autofree char *xml = NULL;
>       const char *device_value = NULL;
> +    unsigned int flags = 0;
>       bool wrap = vshCommandOptBool(cmd, "wrap");
>       const char *xpath = NULL;
>   
> @@ -608,7 +613,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
>       if (!device)
>           return false;
>   
> -    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
> +    if (vshCommandOptBool(cmd, "inactive")) {
> +        flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
> +        if (!virNodeDeviceIsPersistent(device)) {
> +            vshError(ctl, _("Node device '%1$s' is not persistent"), device_value);
> +            return false;
> +        }

I believe you've already handled this within virNodeDeviceGetXMLDesc() 
in patch 4. There shouldn't be any need to duplicate that check here.

> +    }
> +
> +    if (!(xml = virNodeDeviceGetXMLDesc(device, flags)))
>           return false;
>   
>       return virshDumpXML(ctl, xml, "node-device", xpath, wrap);

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 05/11] tools: add option inactive to nodedev-dumpxml
Posted by Boris Fiuczynski 7 months, 1 week ago
On 1/31/24 22:34, Jonathon Jongsma wrote:
> On 1/19/24 10:38 AM, Boris Fiuczynski wrote:
>> Allow to dump the XML of the persisted mdev when the mdev has been
>> started instead of the current state only.
>>
>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   docs/manpages/virsh.rst |  7 +++++--
>>   tools/virsh-nodedev.c   | 15 ++++++++++++++-
>>   2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
>> index ed1027e133..3a814dccd2 100644
>> --- a/docs/manpages/virsh.rst
>> +++ b/docs/manpages/virsh.rst
>> @@ -5422,14 +5422,17 @@ nodedev-dumpxml
>>   ::
>> -   nodedev-dumpxml [--xpath EXPRESSION] [--wrap] device
>> +   nodedev-dumpxml [--inactive] [--xpath EXPRESSION] [--wrap] device
>>   Dump a <device> XML representation for the given node device, including
>>   such information as the device name, which bus owns the device, the
>>   vendor and product id, and any capabilities of the device usable by
>>   libvirt (such as whether device reset is supported). *device* can
>>   be either device name or wwn pair in "wwnn,wwpn" format (only works
>> -for HBA).
>> +for HBA). An additional option affecting the XML dump may be
>> +used. *--inactive* tells virsh to dump the node device configuration
>> +that will be used on next start of the node device as opposed to the
>> +current node device configuration.
>>   If the **--xpath** argument provides an XPath expression, it will be
>>   evaluated against the output XML and only those matching nodes will
>> diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
>> index fb38fd7fcc..54e192d619 100644
>> --- a/tools/virsh-nodedev.c
>> +++ b/tools/virsh-nodedev.c
>> @@ -575,6 +575,10 @@ static const vshCmdOptDef 
>> opts_node_device_dumpxml[] = {
>>        .help = N_("device name or wwn pair in 'wwnn,wwpn' format"),
>>        .completer = virshNodeDeviceNameCompleter,
>>       },
>> +    {.name = "inactive",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("show inactive defined XML"),
>> +    },
>>       {.name = "xpath",
>>        .type = VSH_OT_STRING,
>>        .flags = VSH_OFLAG_REQ_OPT,
>> @@ -594,6 +598,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd 
>> *cmd)
>>       g_autoptr(virshNodeDevice) device = NULL;
>>       g_autofree char *xml = NULL;
>>       const char *device_value = NULL;
>> +    unsigned int flags = 0;
>>       bool wrap = vshCommandOptBool(cmd, "wrap");
>>       const char *xpath = NULL;
>> @@ -608,7 +613,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const 
>> vshCmd *cmd)
>>       if (!device)
>>           return false;
>> -    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
>> +    if (vshCommandOptBool(cmd, "inactive")) {
>> +        flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>> +        if (!virNodeDeviceIsPersistent(device)) {
>> +            vshError(ctl, _("Node device '%1$s' is not persistent"), 
>> device_value);
>> +            return false;
>> +        }
> 
> I believe you've already handled this within virNodeDeviceGetXMLDesc() 
> in patch 4. There shouldn't be any need to duplicate that check here.
> 

Without the check in the client the error message looks like this:
# virsh nodedev-dumpxml --inactive 
mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008
error: Requested operation is not valid: node device 
'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' is not persistent

I added the additional check in the virsh client to create a user 
friendlier message looking like this:
# virsh nodedev-dumpxml --inactive 
mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008
error: Node device 'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' 
is not persistent

Do you want it removed?

>> +    }
>> +
>> +    if (!(xml = virNodeDeviceGetXMLDesc(device, flags)))
>>           return false;
>>       return virshDumpXML(ctl, xml, "node-device", xpath, wrap);
> 
> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Thanks

> _______________________________________________
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-leave@lists.libvirt.org

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 05/11] tools: add option inactive to nodedev-dumpxml
Posted by Jonathon Jongsma 7 months, 1 week ago
On 2/1/24 8:35 AM, Boris Fiuczynski wrote:
> On 1/31/24 22:34, Jonathon Jongsma wrote:
>> On 1/19/24 10:38 AM, Boris Fiuczynski wrote:
>>> Allow to dump the XML of the persisted mdev when the mdev has been
>>> started instead of the current state only.
>>>
>>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>>> ---
>>>   docs/manpages/virsh.rst |  7 +++++--
>>>   tools/virsh-nodedev.c   | 15 ++++++++++++++-
>>>   2 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
>>> index ed1027e133..3a814dccd2 100644
>>> --- a/docs/manpages/virsh.rst
>>> +++ b/docs/manpages/virsh.rst
>>> @@ -5422,14 +5422,17 @@ nodedev-dumpxml
>>>   ::
>>> -   nodedev-dumpxml [--xpath EXPRESSION] [--wrap] device
>>> +   nodedev-dumpxml [--inactive] [--xpath EXPRESSION] [--wrap] device
>>>   Dump a <device> XML representation for the given node device, 
>>> including
>>>   such information as the device name, which bus owns the device, the
>>>   vendor and product id, and any capabilities of the device usable by
>>>   libvirt (such as whether device reset is supported). *device* can
>>>   be either device name or wwn pair in "wwnn,wwpn" format (only works
>>> -for HBA).
>>> +for HBA). An additional option affecting the XML dump may be
>>> +used. *--inactive* tells virsh to dump the node device configuration
>>> +that will be used on next start of the node device as opposed to the
>>> +current node device configuration.
>>>   If the **--xpath** argument provides an XPath expression, it will be
>>>   evaluated against the output XML and only those matching nodes will
>>> diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
>>> index fb38fd7fcc..54e192d619 100644
>>> --- a/tools/virsh-nodedev.c
>>> +++ b/tools/virsh-nodedev.c
>>> @@ -575,6 +575,10 @@ static const vshCmdOptDef 
>>> opts_node_device_dumpxml[] = {
>>>        .help = N_("device name or wwn pair in 'wwnn,wwpn' format"),
>>>        .completer = virshNodeDeviceNameCompleter,
>>>       },
>>> +    {.name = "inactive",
>>> +     .type = VSH_OT_BOOL,
>>> +     .help = N_("show inactive defined XML"),
>>> +    },
>>>       {.name = "xpath",
>>>        .type = VSH_OT_STRING,
>>>        .flags = VSH_OFLAG_REQ_OPT,
>>> @@ -594,6 +598,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const 
>>> vshCmd *cmd)
>>>       g_autoptr(virshNodeDevice) device = NULL;
>>>       g_autofree char *xml = NULL;
>>>       const char *device_value = NULL;
>>> +    unsigned int flags = 0;
>>>       bool wrap = vshCommandOptBool(cmd, "wrap");
>>>       const char *xpath = NULL;
>>> @@ -608,7 +613,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const 
>>> vshCmd *cmd)
>>>       if (!device)
>>>           return false;
>>> -    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
>>> +    if (vshCommandOptBool(cmd, "inactive")) {
>>> +        flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>>> +        if (!virNodeDeviceIsPersistent(device)) {
>>> +            vshError(ctl, _("Node device '%1$s' is not persistent"), 
>>> device_value);
>>> +            return false;
>>> +        }
>>
>> I believe you've already handled this within virNodeDeviceGetXMLDesc() 
>> in patch 4. There shouldn't be any need to duplicate that check here.
>>
> 
> Without the check in the client the error message looks like this:
> # virsh nodedev-dumpxml --inactive 
> mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008
> error: Requested operation is not valid: node device 
> 'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' is not persistent
> 
> I added the additional check in the virsh client to create a user 
> friendlier message looking like this:
> # virsh nodedev-dumpxml --inactive 
> mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008
> error: Node device 'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' 
> is not persistent
> 
> Do you want it removed?

I think the first error message is fine, and it's more maintainable to 
not keep the precondition checks within the function itself.

> 
>>> +    }
>>> +
>>> +    if (!(xml = virNodeDeviceGetXMLDesc(device, flags)))
>>>           return false;
>>>       return virshDumpXML(ctl, xml, "node-device", xpath, wrap);
>>
>> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
> 
> Thanks
> 
>> _______________________________________________
>> Devel mailing list -- devel@lists.libvirt.org
>> To unsubscribe send an email to devel-leave@lists.libvirt.org
> 
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 05/11] tools: add option inactive to nodedev-dumpxml
Posted by Boris Fiuczynski 7 months, 1 week ago
On 2/1/24 17:39, Jonathon Jongsma wrote:
>>>> @@ -608,7 +613,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const 
>>>> vshCmd *cmd)
>>>>       if (!device)
>>>>           return false;
>>>> -    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
>>>> +    if (vshCommandOptBool(cmd, "inactive")) {
>>>> +        flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>>>> +        if (!virNodeDeviceIsPersistent(device)) {
>>>> +            vshError(ctl, _("Node device '%1$s' is not 
>>>> persistent"), device_value);
>>>> +            return false;
>>>> +        }
>>>
>>> I believe you've already handled this within 
>>> virNodeDeviceGetXMLDesc() in patch 4. There shouldn't be any need to 
>>> duplicate that check here.
>>>
>>
>> Without the check in the client the error message looks like this:
>> # virsh nodedev-dumpxml --inactive 
>> mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008
>> error: Requested operation is not valid: node device 
>> 'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' is not persistent
>>
>> I added the additional check in the virsh client to create a user 
>> friendlier message looking like this:
>> # virsh nodedev-dumpxml --inactive 
>> mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008
>> error: Node device 
>> 'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' is not persistent
>>
>> Do you want it removed?
> 
> I think the first error message is fine, and it's more maintainable to 
> not keep the precondition checks within the function itself.
> 

Ok, I removed the precheck.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org