On 1/31/24 22:03, Jonathon Jongsma wrote:
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@xxxxxxxxxxxxx> --- 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.cindex 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.
Changed.
{ 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()
Accepted and changed.
+ 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.
I will remove this change.What I meant to express was that this method is not a simple copy but a surgical copy which eliminates all differences (above called "any change") in the provided destination config.
Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> _______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
-- 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@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
