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

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