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

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx>
>
> Thanks
>
> > _______________________________________________
> > Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
> > To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx