Re: [PATCH v3 12/12] nodedev: allow modify on define of a persistent node device

On 2/16/24 8:52 AM, Boris Fiuczynski wrote: > Allow to modify a node device by using virNodeDeviceDefineXML() to align > its behavior with other drivers define methods. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- >   NEWS.rst                             |  5 +++ >   src/libvirt-nodedev.c                |  4 +- >   src/node_device/node_device_driver.c | 64 ++++++++++++++++++++++------ >   tools/virsh-nodedev.c                |  8 ++-- >   4 files changed, 64 insertions(+), 17 deletions(-) > > diff --git a/NEWS.rst b/NEWS.rst > index aa2fee3533..9b8b94dea4 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -32,6 +32,11 @@ v10.1.0 (unreleased) >   * **Improvements** > +* nodedev: Add ability to update persistent mediated devices by > defining them > + > +    Existing persistent mediated devices can now also be updated by > +    ``virNodeDeviceDefineXML()`` as long as parent and UUID remain > unchanged. > + >   * **Bug fixes** >     * qemu_process: Skip over non-virtio non-TAP NIC models when > refreshing rx-filter > diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c > index b97c199a3a..f242c0a8f6 100644 > --- a/src/libvirt-nodedev.c > +++ b/src/libvirt-nodedev.c > @@ -780,7 +780,9 @@ virNodeDeviceDestroy(virNodeDevicePtr dev) >    * @xmlDesc: string containing an XML description of the device to > be defined >    * @flags: bitwise-OR of supported virNodeDeviceDefineXMLFlags >    * > - * Define a new device on the VM host machine, for example, a > mediated device > + * Define a new inactive persistent device or modify an existing > persistent > + * one from the XML description on the VM host machine, for example, > a mediated > + * device. >    * >    * virNodeDeviceFree should be used to free the resources after the >    * node device object is no longer needed. > diff --git a/src/node_device/node_device_driver.c > b/src/node_device/node_device_driver.c > index 11bc9af92e..66de46513d 100644 > --- a/src/node_device/node_device_driver.c > +++ b/src/node_device/node_device_driver.c > @@ -1544,12 +1544,33 @@ > nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def, >   } > +static virNodeDeviceObj* > +findPersistentMdevNodeDevice(virNodeDeviceDef *def) > +{ > +    virNodeDeviceObj *obj = NULL; > + > +    if (!nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV) && > !def->parent) > +        return NULL; These checks are duplicating checks that already exist in nodeDeviceDefineXML(). If we remove them, I'm not sure it's even worth factoring this out into a separate function.

> + > +    if (def->caps->data.mdev.uuid && > def->caps->data.mdev.parent_addr) { > + > mdevGenerateDeviceName(def); I don't like the fact that calling this function has the side-effect of (potentially) changing the name of the device definition by calling mdevGenerateDeviceName(). While you're only currently calling this function from one location where this won't cause any problems, I'd rather not make assumptions within the function. It'd be better to make sure that the name is set before calling this function (if you keep this as a separate function). Or better yet, just use virNodeDeviceObjListFindMediatedDeviceByUUID() so you don't even need to generate a name for lookup. > +        if ((obj = nodeDeviceObjFindByName(def->name)) && > +            virNodeDeviceObjIsPersistent(obj))

> +            return obj; > +    } > + > +    return NULL; > +} > + > + >   virNodeDevice* >   nodeDeviceDefineXML(virConnect *conn, >                       const char *xmlDesc, >                       unsigned int flags) >   { >       g_autoptr(virNodeDeviceDef) def = NULL; > +    virNodeDevicePtr new_nodedev = NULL; > +    virNodeDeviceObj *persistent_obj = NULL; >       const char *virt_type = NULL; >       g_autofree char *uuid = NULL; >       g_autofree char *name = NULL; > @@ -1566,28 +1587,43 @@ nodeDeviceDefineXML(virConnect *conn, >                                         &driver->parserCallbacks, > NULL, validate))) >           return NULL; > +    persistent_obj = findPersistentMdevNodeDevice(def); > + >       if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0) > -        return NULL; > +        goto cleanup; >       if (!nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { >           virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >                          _("Unsupported device type")); > -        return NULL; > +        goto cleanup; >       } >       if (!def->parent) { >           virReportError(VIR_ERR_XML_ERROR, "%s", >                          _("cannot define a mediated device without a > parent")); > -        return NULL; > +        goto cleanup; >       } > -    if (virMdevctlDefine(def, &uuid) < 0) { > -        return NULL; > -    } > +    if (persistent_obj) { > +        // virNodeDeviceObjUpdateModificationImpact() is not required > we will > +        // modify the persitent config only. typo: persitent -> persistent

> +        // nodeDeviceDefValidateUpdate() is not required as uuid and > parent are > +        // machting if def was found and changing the type in the > persistent typo: machting -> matching

> +        // config is allowed. > +        VIR_DEBUG("Update node device '%s' with mdevctl", def->name); > +        if (virMdevctlModify(def, true, false) < 0) > +            goto cleanup; > -    if (uuid && uuid[0]) { > -        g_free(def->caps->data.mdev.uuid); > -        def->caps->data.mdev.uuid = g_steal_pointer(&uuid); > +        virNodeDeviceObjEndAPI(&persistent_obj); You're calling EndAPI() here and then calling it in the cleanup label again later. Although that won't cause any problems (since the peristent_obj variable will be set to NULL by the first call), it

doesn't feel quite right. One way to avoid this might be to move the findPersistentMdevNodeDevice() call after all of the other checks so that you no longer need the cleanup label below.

> +    } else { > +        VIR_DEBUG("Define node device '%s' with mdevctl", def->name); > +        if (virMdevctlDefine(def, &uuid) < 0) > +            goto cleanup; > + > +        if (uuid && uuid[0]) { > +            g_free(def->caps->data.mdev.uuid); > +            def->caps->data.mdev.uuid = g_steal_pointer(&uuid); > +        } >       } >       mdevGenerateDeviceName(def); > @@ -1601,9 +1637,13 @@ nodeDeviceDefineXML(virConnect *conn, >        * add the provisional device to the list and return it > immediately and >        * avoid this long delay. */ >       if (nodeDeviceUpdateMediatedDevice(g_steal_pointer(&def), true) > < 0) > -        return NULL; > +        goto cleanup; > + > +    new_nodedev = virGetNodeDevice(conn, name); > -    return virGetNodeDevice(conn, name); > + cleanup: > +    virNodeDeviceObjEndAPI(&persistent_obj); > +    return new_nodedev; >   } > @@ -2237,7 +2277,7 @@ nodeDeviceDefValidateUpdate(virNodeDeviceDef *def, >                          cap_mdev->uuid); >           return -1; >       } > - > + >       /* A live update cannot change the mdev type. Since the new > config is >        * stored in defined_config, compare that to the mdev type of > the current >        * live config to make sure they match */ > diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c > index 7b80747819..912975dfed 100644 > --- a/tools/virsh-nodedev.c > +++ b/tools/virsh-nodedev.c > @@ -1083,12 +1083,12 @@ cmdNodeDeviceUndefine(vshControl *ctl, const > vshCmd *cmd G_GNUC_UNUSED) >    */ >   static const vshCmdInfo info_node_device_define[] = { >       {.name = "help", > -     .data = N_("Define a device by an xml file on a node") > +     .data = N_("Define or modify a device by an xml file on a node") >       }, >       {.name = "desc", > -     .data = N_("Defines a persistent device on the node that can be " > -                "assigned to a domain. The device must be started > before " > -                "it can be assigned to a domain.") > +     .data = N_("Defines or modifies a persistent device on the node > that " > +                "can be assigned to a domain. The device must be > started " > +                "before it can be assigned to a domain.") >       }, >       {.name = NULL} >   }; _______________________________________________ Devel mailing list -- devel(a)lists.libvirt.org To unsubscribe send an email to devel-leave(a)lists.libvirt.org