Re: [PATCH 02/11] node_device: refactor mdev attributes handling

On 1/19/24 10:38 AM, Boris Fiuczynski wrote: > Refactor attribute handling code into methods for easier reuse. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- >   src/conf/node_device_conf.c          |  27 ++++--- >   src/node_device/node_device_driver.c | 108 ++++++++++++++++----------- >   2 files changed, 83 insertions(+), 52 deletions(-) > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 4e1bde503f..68924b3027 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -585,11 +585,22 @@ virNodeDeviceCapStorageDefFormat(virBuffer *buf, >   } >   static void > -virNodeDeviceCapMdevDefFormat(virBuffer *buf, > -                              const virNodeDevCapData *data) > +virNodeDeviceCapMdevAttrFormat(virBuffer *buf, > +                               const virMediatedDeviceConfig *config) >   { >       size_t i; > +    for (i = 0; i < config->nattributes; i++) { > +        virMediatedDeviceAttr *attr = config->attributes[i]; > +        virBufferAsprintf(buf, "<attr name='%s' value='%s'/>\n", > +                          attr->name, attr->value); > +    } > +} > + > +static void > +virNodeDeviceCapMdevDefFormat(virBuffer *buf, > +                              const virNodeDevCapData *data) > +{ >       virBufferEscapeString(buf, "<type id='%s'/>\n", > data->mdev.dev_config.type); >       virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid); >       virBufferEscapeString(buf, "<parent_addr>%s</parent_addr>\n", > @@ -597,11 +608,7 @@ virNodeDeviceCapMdevDefFormat(virBuffer *buf, >       virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n", >                         data->mdev.iommuGroupNumber); > -    for (i = 0; i < data->mdev.dev_config.nattributes; i++) { > -        virMediatedDeviceAttr *attr = > data->mdev.dev_config.attributes[i]; > -        virBufferAsprintf(buf, "<attr name='%s' value='%s'/>\n", > -                          attr->name, attr->value); > -    } > +    virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.dev_config); >   } >   static void > @@ -2188,7 +2195,7 @@ virNodeDevCapSystemParseXML(xmlXPathContextPtr > ctxt, >   static int >   virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt, >                                      xmlNodePtr node, > -                                   virNodeDevCapMdev *mdev) > +                                   virMediatedDeviceConfig *config) >   { >       VIR_XPATH_NODE_AUTORESTORE(ctxt) >       g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew(); > @@ -2202,7 +2209,7 @@ > virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt, >           return -1; >       } > -    VIR_APPEND_ELEMENT(mdev->dev_config.attributes, > mdev->dev_config.nattributes, attr); > +    VIR_APPEND_ELEMENT(config->attributes, config->nattributes, attr); >       return 0; >   } > @@ -2253,7 +2260,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, >           return -1; >       for (i = 0; i < nattrs; i++) > -        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], mdev); > +        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], > &mdev->dev_config); >       return 0; >   } > diff --git a/src/node_device/node_device_driver.c > b/src/node_device/node_device_driver.c > index 1ee59d710b..0c8612eb71 100644 > --- a/src/node_device/node_device_driver.c > +++ b/src/node_device/node_device_driver.c > @@ -599,27 +599,16 @@ nodeDeviceHasCapability(virNodeDeviceDef *def, > virNodeDevCapType type) >   } > -/* format a json string that provides configuration information about > this mdev > - * to the mdevctl utility */ >   static int > -nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf) > +nodeDeviceAttributesToJSON(virJSONValue *json, > +                           const virMediatedDeviceConfig config) I don't see any compelling reason to pass this config struct by value. Just pass a pointer.

>   { >       size_t i; > -    virNodeDevCapMdev *mdev = &def->caps->data.mdev; > -    g_autoptr(virJSONValue) json = virJSONValueNewObject(); > -    const char *startval = mdev->autostart ? "auto" : "manual"; > - > -    if (virJSONValueObjectAppendString(json, "mdev_type", > mdev->dev_config.type) < 0) > -        return -1; > - > -    if (virJSONValueObjectAppendString(json, "start", startval) < 0) > -        return -1; > - > -    if (mdev->dev_config.attributes) { > +    if (config.attributes) { >           g_autoptr(virJSONValue) attributes = virJSONValueNewArray(); > -        for (i = 0; i < mdev->dev_config.nattributes; i++) { > -            virMediatedDeviceAttr *attr = > mdev->dev_config.attributes[i]; > +        for (i = 0; i < config.nattributes; i++) { > +            virMediatedDeviceAttr *attr = config.attributes[i]; >               g_autoptr(virJSONValue) jsonattr = virJSONValueNewObject(); >               if (virJSONValueObjectAppendString(jsonattr, attr->name, > attr->value) < 0) > @@ -633,6 +622,28 @@ nodeDeviceDefToMdevctlConfig(virNodeDeviceDef > *def, char **buf) >               return -1; >       } > +    return 0; > +} > + > + > +/* format a json string that provides configuration information about > this mdev > + * to the mdevctl utility */ > +static int > +nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf) > +{ > +    virNodeDevCapMdev *mdev = &def->caps->data.mdev; > +    g_autoptr(virJSONValue) json = virJSONValueNewObject(); > +    const char *startval = mdev->autostart ? "auto" : "manual"; > + > +    if (virJSONValueObjectAppendString(json, "mdev_type", > mdev->dev_config.type) < 0) > +        return -1; > + > +    if (virJSONValueObjectAppendString(json, "start", startval) < 0) > +        return -1; > + > +    if (nodeDeviceAttributesToJSON(json, mdev->dev_config) < 0) > +        return -1; > + >       *buf = virJSONValueToString(json, false); >       if (!*buf) >           return -1; > @@ -1092,6 +1103,39 @@ matchDeviceAddress(virNodeDeviceObj *obj, >   } > +static int > +nodeDeviceParseMdevctlAttribs(virMediatedDeviceConfig *config, As far as I can tell, we don't use the abbreviation "attribs" anywhere else in libvirt, but we do use both 'Attributes' and 'Attrs'. For consistency, I'd suggest just using the full word: nodeDeviceParseMdevctlAttributes()

> +                              virJSONValue *attrs) > +{ > +    size_t i; > + > +    if (attrs && virJSONValueIsArray(attrs)) { > +        int nattrs = virJSONValueArraySize(attrs); > + > +        config->attributes = g_new0(virMediatedDeviceAttr*, nattrs); > +        config->nattributes = nattrs; > + > +        for (i = 0; i < nattrs; i++) { > +            virJSONValue *attr = virJSONValueArrayGet(attrs, i); > +            virMediatedDeviceAttr *attribute; > +            virJSONValue *value; > + > +            if (!virJSONValueIsObject(attr) || > +                virJSONValueObjectKeysNumber(attr) != 1) > +                return -1; > + > +            attribute = g_new0(virMediatedDeviceAttr, 1); > +            attribute->name = g_strdup(virJSONValueObjectGetKey(attr, > 0)); > +            value = virJSONValueObjectGetValue(attr, 0); > +            attribute->value = g_strdup(virJSONValueGetString(value)); > +            config->attributes[i] = attribute; > +        } > +    } > + > +    return 0; > +} > + > + >   static virNodeDeviceDef* >   nodeDeviceParseMdevctlChildDevice(const char *parent, >                                     virJSONValue *json) > @@ -1099,7 +1143,6 @@ nodeDeviceParseMdevctlChildDevice(const char > *parent, >       virNodeDevCapMdev *mdev; >       const char *uuid; >       virJSONValue *props; > -    virJSONValue *attrs; >       g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); >       virNodeDeviceObj *parent_obj; >       const char *start = NULL; > @@ -1134,31 +1177,10 @@ nodeDeviceParseMdevctlChildDevice(const char > *parent, >       start = virJSONValueObjectGetString(props, "start"); >       mdev->autostart = STREQ_NULLABLE(start, "auto"); > -    attrs = virJSONValueObjectGet(props, "attrs"); > - > -    if (attrs && virJSONValueIsArray(attrs)) { > -        size_t i; > -        int nattrs = virJSONValueArraySize(attrs); > - > -        mdev->dev_config.attributes = g_new0(virMediatedDeviceAttr*, > nattrs); > -        mdev->dev_config.nattributes = nattrs; > - > -        for (i = 0; i < nattrs; i++) { > -            virJSONValue *attr = virJSONValueArrayGet(attrs, i); > -            virMediatedDeviceAttr *attribute; > -            virJSONValue *value; > - > -            if (!virJSONValueIsObject(attr) || > -                virJSONValueObjectKeysNumber(attr) != 1) > -                return NULL; > +    if (nodeDeviceParseMdevctlAttribs(&mdev->dev_config, > +                                      virJSONValueObjectGet(props, > "attrs")) < 0) > +        return NULL; > -            attribute = g_new0(virMediatedDeviceAttr, 1); > -            attribute->name = g_strdup(virJSONValueObjectGetKey(attr, > 0)); > -            value = virJSONValueObjectGetValue(attr, 0); > -            attribute->value = g_strdup(virJSONValueGetString(value)); > -            mdev->dev_config.attributes[i] = attribute; > -        } > -    } >       mdevGenerateDeviceName(child); >       return g_steal_pointer(&child); > @@ -1760,7 +1782,9 @@ nodeDeviceUpdateMediatedDevices(void) >   } > -/* returns true if any attributes were copied, else returns false */ > +/* Transfer mediated device attributes to the new definition. Any > change in > + * the attributes is elminated but causes the return value to be true. > + * Returns true if any attribute is copied, else returns false */ This change doesn't really seem to belong in this commit as there are no changes to this function. Also, I don't understand the comment. I suppose "elminated" is supposed to be "eliminated", but I still can't quite understand what it's trying to say.

Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; _______________________________________________ Devel mailing list -- devel(a)lists.libvirt.org To unsubscribe send an email to devel-leave(a)lists.libvirt.org