Refactor attribute handling code into methods for easier reuse.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
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)
{
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,
+ 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 */
static bool
virMediatedDeviceAttrsCopy(virMediatedDeviceConfig *dst_config,
virMediatedDeviceConfig *src_config)
--
2.42.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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@linux.ibm.com>
> ---
> 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@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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@linux.ibm.com>
>> ---
>> 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.
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@redhat.com>
> _______________________________________________
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-leave@lists.libvirt.org
--
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@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2024 Red Hat, Inc.