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>

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:

The configuration of a defined mdev can be modified after the mdev is started. The defined configuration and the active configuration can therefore run out of sync. Handle this by storing the modifiable data which is the mdev type and attributes in two separate active and defined configurations. mdevctl supports with callout scripts to do an attribute retrieval of started mdevs which is already implemented in libvirt.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- include/libvirt/libvirt-nodedev.h | 11 ++++ src/conf/node_device_conf.c | 53 ++++++++++------ src/conf/node_device_conf.h | 5 +- src/libvirt-nodedev.c | 2 +- src/node_device/node_device_driver.c | 61 +++++++++++++------ src/node_device/node_device_driver.h | 6 +- src/node_device/node_device_udev.c | 4 +- src/test/test_driver.c | 6 +- tests/nodedevmdevctltest.c | 4 +- ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 14 +++++ ...d_b7f0_4fea_b468_f1da537d301b_inactive.xml | 1 + ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 10 +++ ...c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml | 9 +++ ...9_36ea_4111_8f0a_8c9a70e21366_inactive.xml | 1 + ...9_495e_4243_ad9f_beb3f14c23d9_inactive.xml | 1 + ...4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml | 8 +++ ...6_1ca8_49ac_b176_871d16c13076_inactive.xml | 1 + tests/nodedevxml2xmltest.c | 59 +++++++++++++++--- 18 files changed, 197 insertions(+), 59 deletions(-) create mode 100644 tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml create mode 120000 tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml create mode 120000 tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml create mode 120000 tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml create mode 100644 tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml create mode 120000 tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml

diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 428b0d722f..d18246e2f6 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -117,6 +117,17 @@ int virNodeDeviceListCaps (virNodeDevicePtr dev, char **const names, int maxnames);

+/** + * virNodeDeviceGetXMLDescFlags: + * + * Flags used to provide the state of the returned node device configuration. + * + * Since: 10.1.0 + */ +typedef enum { + VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE = 1 << 0, /* dump inactive device configuration (Since: 10.1.0) */ +} virNodeDeviceGetXMLDescFlags;

In all of the other similar cases, this type is named vir$(OBJECT)XMLFlags and the flag itself is named VIR_$(OBJECT)_XML_INACTIVE. So for consistency, I'd remove the 'get' and the 'desc' from the names.

+ char * virNodeDeviceGetXMLDesc (virNodeDevicePtr dev, unsigned int flags);

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 9a0fc68e67..40e15ef4da 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -599,16 +599,22 @@ virNodeDeviceCapMdevAttrFormat(virBuffer *buf,

static void virNodeDeviceCapMdevDefFormat(virBuffer *buf, - const virNodeDevCapData *data) + const virNodeDevCapData *data, + bool defined) { - virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.dev_config.type); + if (defined) + virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.defined_config.type); + else + virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.active_config.type); virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid); virBufferEscapeString(buf, "<parent_addr>%s</parent_addr>\n", data->mdev.parent_addr); virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n", data->mdev.iommuGroupNumber); - - virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.dev_config); + if (defined) + virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.defined_config); + else + virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.active_config); }

static void @@ -659,25 +665,28 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,

char * -virNodeDeviceDefFormat(const virNodeDeviceDef *def) +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virNodeDevCapsDef *caps; size_t i = 0; + bool inactive_state = flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;

virBufferAddLit(&buf, "<device>\n"); virBufferAdjustIndent(&buf, 2); virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); - virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path); - virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n", - def->devnode); - if (def->devlinks) { - for (i = 0; def->devlinks[i]; i++) - virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n", - def->devlinks[i]); + if (!inactive_state) { + virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path); + virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n", + def->devnode); + if (def->devlinks) { + for (i = 0; def->devlinks[i]; i++) + virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n", + def->devlinks[i]); + } } virBufferEscapeString(&buf, "<parent>%s</parent>\n", def->parent); - if (def->driver) { + if (def->driver && !inactive_state) { virBufferAddLit(&buf, "<driver>\n"); virBufferAdjustIndent(&buf, 2); virBufferEscapeString(&buf, "<name>%s</name>\n", def->driver); @@ -738,7 +747,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type)); break; case VIR_NODE_DEV_CAP_MDEV: - virNodeDeviceCapMdevDefFormat(&buf, data); + virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state); break; case VIR_NODE_DEV_CAP_CCW_DEV: virNodeDeviceCapCCWDefFormat(&buf, data); @@ -2226,7 +2235,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,

ctxt->node = node;

- if (!(mdev->dev_config.type = virXPathString("string(./type[1]/@id)", ctxt))) { + if (!(mdev->defined_config.type = virXPathString("string(./type[1]/@id)", ctxt))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("missing type id attribute for '%1$s'"), def->name); return -1; @@ -2258,7 +2267,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, return -1;

for (i = 0; i < nattrs; i++) - virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->dev_config); + virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->defined_config);

return 0; } @@ -2601,11 +2610,15 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps) g_free(data->sg.path); break; case VIR_NODE_DEV_CAP_MDEV: - g_free(data->mdev.dev_config.type); + g_free(data->mdev.defined_config.type); + g_free(data->mdev.active_config.type); g_free(data->mdev.uuid); - for (i = 0; i < data->mdev.dev_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.dev_config.attributes[i]); - g_free(data->mdev.dev_config.attributes); + for (i = 0; i < data->mdev.defined_config.nattributes; i++) + virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]); + g_free(data->mdev.defined_config.attributes); + for (i = 0; i < data->mdev.active_config.nattributes; i++) + virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]); + g_free(data->mdev.active_config.attributes); g_free(data->mdev.parent_addr); break; case VIR_NODE_DEV_CAP_CSS_DEV: diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index f100496272..f59440dbb9 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -153,7 +153,8 @@ typedef struct _virNodeDevCapMdev virNodeDevCapMdev; struct _virNodeDevCapMdev { unsigned int iommuGroupNumber; char *uuid; - virMediatedDeviceConfig dev_config; + virMediatedDeviceConfig defined_config; + virMediatedDeviceConfig active_config; char *parent_addr; bool autostart; }; @@ -360,7 +361,7 @@ struct _virNodeDeviceDef { };

char * -virNodeDeviceDefFormat(const virNodeDeviceDef *def); +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags);

typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef *dev, diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index f0f99bc020..9cc5c6466b 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -264,7 +264,7 @@ virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn, /** * virNodeDeviceGetXMLDesc: * @dev: pointer to the node device - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virNodeDeviceGetXMLDescFlags * * Fetch an XML document describing all aspects of * the device. diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0c8612eb71..d67c95d68d 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -338,7 +338,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device, virNodeDeviceDef *def; char *ret = NULL;

- virCheckFlags(0, NULL); + virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);

if (nodeDeviceInitWait() < 0) return NULL; @@ -356,7 +356,19 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device, if (virNodeDeviceUpdateCaps(def) < 0) goto cleanup;

- ret = virNodeDeviceDefFormat(def); + if (flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) { + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("node device '%1$s' is not persistent"), + def->name); + goto cleanup; + } + } else { + if (!virNodeDeviceObjIsActive(obj)) + flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE; + } + + ret = virNodeDeviceDefFormat(def, flags);

cleanup: virNodeDeviceObjEndAPI(&obj); @@ -629,19 +641,20 @@ nodeDeviceAttributesToJSON(virJSONValue *json, /* format a json string that provides configuration information about this mdev * to the mdevctl utility */ static int -nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf) +nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf, bool defined)

As far as I can tell, you never actually call this function with 'defined' set to false. It doesn't seem to be used in any of the following commits either.

{ virNodeDevCapMdev *mdev = &def->caps->data.mdev; + virMediatedDeviceConfig mdev_config = defined ? mdev->defined_config : mdev->active_config; g_autoptr(virJSONValue) json = virJSONValueNewObject(); const char *startval = mdev->autostart ? "auto" : "manual";

- if (virJSONValueObjectAppendString(json, "mdev_type", mdev->dev_config.type) < 0) + if (virJSONValueObjectAppendString(json, "mdev_type", mdev_config.type) < 0) return -1;

if (virJSONValueObjectAppendString(json, "start", startval) < 0) return -1;

- if (nodeDeviceAttributesToJSON(json, mdev->dev_config) < 0) + if (nodeDeviceAttributesToJSON(json, mdev_config) < 0) return -1;

*buf = virJSONValueToString(json, false); @@ -760,7 +773,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, return NULL; }

- if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) { + if (nodeDeviceDefToMdevctlConfig(def, &inbuf, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("couldn't convert node device def to mdevctl JSON")); return NULL; @@ -1138,9 +1151,11 @@ nodeDeviceParseMdevctlAttribs(virMediatedDeviceConfig *config,

static virNodeDeviceDef* nodeDeviceParseMdevctlChildDevice(const char *parent, - virJSONValue *json) + virJSONValue *json, + bool defined) { virNodeDevCapMdev *mdev; + virMediatedDeviceConfig *mdev_config; const char *uuid; virJSONValue *props; g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); @@ -1170,14 +1185,15 @@ nodeDeviceParseMdevctlChildDevice(const char *parent, child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;

mdev = &child->caps->data.mdev; + mdev_config = defined ? &mdev->defined_config : &mdev->active_config; mdev->uuid = g_strdup(uuid); mdev->parent_addr = g_strdup(parent); - mdev->dev_config.type = + mdev_config->type = g_strdup(virJSONValueObjectGetString(props, "mdev_type")); start = virJSONValueObjectGetString(props, "start"); mdev->autostart = STREQ_NULLABLE(start, "auto");

- if (nodeDeviceParseMdevctlAttribs(&mdev->dev_config, + if (nodeDeviceParseMdevctlAttribs(mdev_config, virJSONValueObjectGet(props, "attrs")) < 0) return NULL;

@@ -1189,7 +1205,8 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,

int nodeDeviceParseMdevctlJSON(const char *jsonstring, - virNodeDeviceDef ***devs) + virNodeDeviceDef ***devs, + bool defined) { int n; g_autoptr(virJSONValue) json_devicelist = NULL; @@ -1259,7 +1276,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring, g_autoptr(virNodeDeviceDef) child = NULL; virJSONValue *child_obj = virJSONValueArrayGet(child_array, j);

- if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) { + if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj, defined))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to parse child device")); goto error; @@ -1402,7 +1419,7 @@ nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def, /* Active devices contain some additional information (e.g. sysfs * path) that is not provided by mdevctl, so re-use the existing * definition and copy over new mdev data */ - changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned, defined);

if (was_defined && !changed) { /* if this device was already defined and the definition @@ -1672,7 +1689,7 @@ virMdevctlList(bool defined, return -1; }

- return nodeDeviceParseMdevctlJSON(output, devs); + return nodeDeviceParseMdevctlJSON(output, devs, defined); }

@@ -1831,16 +1848,24 @@ virMediatedDeviceAttrsCopy(virMediatedDeviceConfig *dst_config, * Returns true if anything was copied, else returns false */ bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, - virNodeDeviceDef *src) + virNodeDeviceDef *src, + bool defined) { bool ret = false; virNodeDevCapMdev *srcmdev = &src->caps->data.mdev; virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev; + virMediatedDeviceConfig *srcmdevconfig = &src->caps->data.mdev.active_config; + virMediatedDeviceConfig *dstmdevconfig = &dst->caps->data.mdev.active_config; + + if (defined) { + srcmdevconfig = &src->caps->data.mdev.defined_config; + dstmdevconfig = &dst->caps->data.mdev.defined_config; + }

- if (STRNEQ_NULLABLE(dstmdev->dev_config.type, srcmdev->dev_config.type)) { + if (STRNEQ_NULLABLE(dstmdevconfig->type, srcmdevconfig->type)) { ret = true; - g_free(dstmdev->dev_config.type); - dstmdev->dev_config.type = g_strdup(srcmdev->dev_config.type); + g_free(dstmdevconfig->type); + dstmdevconfig->type = g_strdup(srcmdevconfig->type); }

if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) { @@ -1849,7 +1874,7 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, dstmdev->uuid = g_strdup(srcmdev->uuid); }

- if (virMediatedDeviceAttrsCopy(&dstmdev->dev_config, &srcmdev->dev_config)) + if (virMediatedDeviceAttrsCopy(dstmdevconfig, srcmdevconfig)) ret = true;

if (dstmdev->autostart != srcmdev->autostart) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index c7d5e22daf..4dce7e6f17 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -142,7 +142,8 @@ nodeDeviceGetMdevctlListCommand(bool defined,

int nodeDeviceParseMdevctlJSON(const char *jsonstring, - virNodeDeviceDef ***devs); + virNodeDeviceDef ***devs, + bool defined);

int nodeDeviceUpdateMediatedDevices(void); @@ -154,7 +155,8 @@ nodeDeviceGenerateName(virNodeDeviceDef *def, const char *s);

bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, - virNodeDeviceDef *src); + virNodeDeviceDef *src, + bool defined);

int nodeDeviceCreate(virNodeDevice *dev, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 254e802c50..57368a96c3 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1069,7 +1069,7 @@ udevProcessMediatedDevice(struct udev_device *dev, return -1; }

- data->dev_config.type = g_path_get_basename(canonicalpath); + data->active_config.type = g_path_get_basename(canonicalpath);

data->uuid = g_strdup(udev_device_get_sysname(dev)); if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0) @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device) objdef = virNodeDeviceObjGetDef(obj);

if (is_mdev) - nodeDeviceDefCopyFromMdevctl(def, objdef); + nodeDeviceDefCopyFromMdevctl(def, objdef, false);

persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ed545848af..01863233bc 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7514,12 +7514,12 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev, virNodeDeviceObj *obj; char *ret = NULL;

- virCheckFlags(0, NULL); + virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);

if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) return NULL;

- ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); + ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);

virNodeDeviceObjEndAPI(&obj); return ret; @@ -7619,7 +7619,7 @@ testNodeDeviceMockCreateVport(testDriver *driver, "scsi_host11"))) goto cleanup;

- xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); + xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy), 0); virNodeDeviceObjEndAPI(&objcopy); if (!xml) goto cleanup; diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index e403328e5a..852d9ed6e7 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -229,13 +229,13 @@ testMdevctlParse(const void *data) return -1; }

- if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) { + if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs, true)) < 0) { VIR_TEST_DEBUG("Unable to parse json for %s", filename); return -1; }

for (i = 0; i < nmdevs; i++) { - g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]); + g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i], VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE); if (!devxml) goto cleanup; virBufferAddStr(&xmloutbuf, devxml); diff --git a/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml new file mode 100644 index 0000000000..6926559efa --- /dev/null +++ b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml @@ -0,0 +1,14 @@ +<device> + <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name> + <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path> + <parent>css_0_0_0052</parent> + <driver> + <name>vfio_ccw_mdev</name> + </driver> + <capability type='mdev'> + <type id='vfio_ccw-io'/> + <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid> + <parent_addr>0.0.0052</parent_addr> + <iommuGroup number='4'/> + </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml new file mode 120000 index 0000000000..f8ec7d8a32 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml @@ -0,0 +1 @@ +mdev_3627463d_b7f0_4fea_b468_f1da537d301b.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml new file mode 100644 index 0000000000..82c60cc065 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml @@ -0,0 +1,10 @@ +<device> + <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name> + <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path> + <parent>css_0_0_0052</parent> + <capability type='mdev'> + <type id='vfio_ccw-io'/> + <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid> + <iommuGroup number='4'/> + </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml new file mode 100644 index 0000000000..87c5ed1340 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml @@ -0,0 +1,9 @@ +<device> + <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name> + <parent>css_0_0_0052</parent> + <capability type='mdev'> + <type id='vfio_ccw-io'/> + <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid> + <iommuGroup number='4'/> + </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml new file mode 120000 index 0000000000..f6d5789399 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml @@ -0,0 +1 @@ +mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml new file mode 120000 index 0000000000..233a7506fb --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml @@ -0,0 +1 @@ +mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml new file mode 100644 index 0000000000..8c4983f43c --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml @@ -0,0 +1,8 @@ +<device> + <name>mdev_ee0b88c4-f554-4dc1-809d-b2a01e8e48ad</name> + <parent>ap_matrix</parent> + <capability type='mdev'> + <type id='vfio_ap-passthrough'/> + <iommuGroup number='0'/> + </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml new file mode 120000 index 0000000000..e053844177 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml @@ -0,0 +1 @@ +mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 068ec68769..be7a5b4df9 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -11,14 +11,20 @@

#define VIR_FROM_THIS VIR_FROM_NONE

+struct TestData { + const char *filename; + unsigned int flags; +}; + static int -testCompareXMLToXMLFiles(const char *xml, const char *outfile) +testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flags) { g_autofree char *xmlData = NULL; g_autofree char *actual = NULL; int ret = -1; virNodeDeviceDef *dev = NULL; virNodeDevCapsDef *caps; + size_t i;

if (virTestLoadFile(xml, &xmlData) < 0) goto fail; @@ -46,9 +52,22 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile) data->storage.logical_block_size; } } + + if (caps->data.type == VIR_NODE_DEV_CAP_MDEV && + !(flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)) { + data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type); + for (i = 0; i < data->mdev.defined_config.nattributes; i++) { + g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1); + attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name); + attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value); + VIR_APPEND_ELEMENT(data->mdev.active_config.attributes, + data->mdev.active_config.nattributes, + attr); + } + } }

- if (!(actual = virNodeDeviceDefFormat(dev))) + if (!(actual = virNodeDeviceDefFormat(dev, flags))) goto fail;

if (virTestCompareToFile(actual, outfile) < 0) @@ -65,16 +84,21 @@ static int testCompareXMLToXMLHelper(const void *data) { int result = -1; + const struct TestData *tdata = data; g_autofree char *xml = NULL; g_autofree char *outfile = NULL;

xml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir, - (const char *)data); + tdata->filename);

- outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir, - (const char *)data); + if (tdata->flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) + outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s_inactive.xml", abs_srcdir, + tdata->filename); + else + outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir, + tdata->filename);

- result = testCompareXMLToXMLFiles(xml, outfile); + result = testCompareXMLToXMLFiles(xml, outfile, tdata->flags);

return result; } @@ -85,10 +109,20 @@ mymain(void) { int ret = 0;

+#define DO_TEST_FLAGS(desc, filename, flags) \ + do { \ + struct TestData data = { filename, flags }; \ + if (virTestRun(desc, testCompareXMLToXMLHelper, &data) < 0) \ + ret = -1; \ + } \ + while (0) + #define DO_TEST(name) \ - if (virTestRun("Node device XML-2-XML " name, \ - testCompareXMLToXMLHelper, (name)) < 0) \ - ret = -1 + DO_TEST_FLAGS("Node device XML-2-XML " name, name, 0) + +#define DO_TEST_INACTIVE(name) \ + DO_TEST_FLAGS("Node device XML-2-XML INACTIVE " name, \ + name, VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)

DO_TEST("computer"); DO_TEST("DVD_GCC_4247N"); @@ -121,6 +155,7 @@ mymain(void) DO_TEST("pci_0000_02_10_7_mdev_types"); DO_TEST("pci_0000_42_00_0_vpd"); DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b"); + DO_TEST_INACTIVE("mdev_3627463d_b7f0_4fea_b468_f1da537d301b"); DO_TEST("ccw_0_0_ffff"); DO_TEST("css_0_0_ffff"); DO_TEST("css_0_0_ffff_channel_dev_addr"); @@ -134,7 +169,13 @@ mymain(void) DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); + DO_TEST_INACTIVE("mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad"); + DO_TEST_INACTIVE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); + DO_TEST_INACTIVE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + DO_TEST_INACTIVE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); DO_TEST("hba_vport_ops"); + DO_TEST("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c"); + DO_TEST_INACTIVE("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");

return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; }

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On 2/1/24 6:17 AM, Boris Fiuczynski wrote:

On 1/31/24 22:30, Jonathon Jongsma wrote:

...

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:

...

The configuration of a defined mdev can be modified after the mdev is started. The defined configuration and the active configuration can therefore run out of sync. Handle this by storing the modifiable data which is the mdev type and attributes in two separate active and defined configurations.  mdevctl supports with callout scripts to do an attribute retrieval of started mdevs which is already implemented in libvirt.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---   include/libvirt/libvirt-nodedev.h             | 11 ++++   src/conf/node_device_conf.c                   | 53 ++++++++++------   src/conf/node_device_conf.h                   |  5 +-   src/libvirt-nodedev.c                         |  2 +-   src/node_device/node_device_driver.c          | 61 +++++++++++++------   src/node_device/node_device_driver.h          |  6 +-   src/node_device/node_device_udev.c            |  4 +-   src/test/test_driver.c                        |  6 +-   tests/nodedevmdevctltest.c                    |  4 +-   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 14 +++++   ...d_b7f0_4fea_b468_f1da537d301b_inactive.xml |  1 +   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 10 +++   ...c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml |  9 +++   ...9_36ea_4111_8f0a_8c9a70e21366_inactive.xml |  1 +   ...9_495e_4243_ad9f_beb3f14c23d9_inactive.xml |  1 +   ...4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml |  8 +++   ...6_1ca8_49ac_b176_871d16c13076_inactive.xml |  1 +   tests/nodedevxml2xmltest.c                    | 59 +++++++++++++++---   18 files changed, 197 insertions(+), 59 deletions(-)   create mode 100644 tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml   create mode 120000 tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml   create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml   create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml   create mode 120000 tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml   create mode 120000 tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml   create mode 100644 tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml   create mode 120000 tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml

diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 428b0d722f..d18246e2f6 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -117,6 +117,17 @@ int                     virNodeDeviceListCaps (virNodeDevicePtr dev,                                                     char **const names,                                                     int maxnames); +/** + * virNodeDeviceGetXMLDescFlags: + * + * Flags used to provide the state of the returned node device configuration. + * + * Since: 10.1.0 + */ +typedef enum { +    VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE            = 1 << 0, /* dump inactive device configuration (Since: 10.1.0) */ +} virNodeDeviceGetXMLDescFlags;

In all of the other similar cases, this type is named vir$(OBJECT)XMLFlags and the flag itself is named VIR_$(OBJECT)_XML_INACTIVE. So for consistency, I'd remove the 'get' and the 'desc' from the names.

I disagree as all other methods that make use of flags base the flags names on the method name. Here are the examples:

virNodeDeviceCreateXML virNodeDeviceCreateXMLFlags vir Node Device Create XML VIR_NODE_DEVICE_CREATE_XML_*

virNodeDeviceDefineXML virNodeDeviceDefineXMLFlags vir Node Device Define XML VIR_NODE_DEVICE_DEFINE_XML_*

virConnectListAllNodeDevices virConnectListAllNodeDeviceFlags vir Connect List Node Device [All is removed] VIR_CONNECT_LIST_NODE_DEVICES_*

These are the reasons I chose for consistency: virNodeDeviceGetXMLDesc virNodeDeviceGetXMLDescFlags vir Node Device Get XML Desc VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE

That's true in general, however for the *GetXMLDesc() functions, this pattern doesn't hold: domain: - virDomainGetXMLDesc() - virDomainXMLFlags - VIR_DOMAIN_XML_INACTIVE storage: - virStoragePoolGetXMLDesc() - virStorageXMLFlags - VIR_STORAGE_XML_INACTIVE network: - virNetworkGetXMLDesc() - virNetworkXMLFlags - VIR_NETWORK_XML_INACTIVE interface: - virInterfaceGetXMLDesc() - virInterfaceXMLFlags - VIR_INTERFACE_XML_INACTIVE There are no types following the *GetXMLDescFlags pattern: $ git grep XMLDescFlags include/ $ I don't feel personally strongly about this point because there are consistency arguments for both approaches. But I thought I'd mention it. Jonathon

...

...

+   char *                  virNodeDeviceGetXMLDesc (virNodeDevicePtr dev,                                                    unsigned int flags); diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 9a0fc68e67..40e15ef4da 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -599,16 +599,22 @@ virNodeDeviceCapMdevAttrFormat(virBuffer *buf,   static void   virNodeDeviceCapMdevDefFormat(virBuffer *buf, -                              const virNodeDevCapData *data) +                              const virNodeDevCapData *data, +                              bool defined)   { -    virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.dev_config.type); +    if (defined) +        virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.defined_config.type); +    else +        virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.active_config.type);       virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid);       virBufferEscapeString(buf, "<parent_addr>%s</parent_addr>\n",                             data->mdev.parent_addr);       virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",                         data->mdev.iommuGroupNumber); - -    virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.dev_config); +    if (defined) +        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.defined_config); +    else +        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.active_config);   }   static void @@ -659,25 +665,28 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,   char * -virNodeDeviceDefFormat(const virNodeDeviceDef *def) +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags)   {       g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;       virNodeDevCapsDef *caps;       size_t i = 0; +    bool inactive_state = flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;       virBufferAddLit(&buf, "<device>\n");       virBufferAdjustIndent(&buf, 2);       virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); -    virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path); -    virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n", -                          def->devnode); -    if (def->devlinks) { -        for (i = 0; def->devlinks[i]; i++) -            virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n", -                                  def->devlinks[i]); +    if (!inactive_state) { +        virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path); +        virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n", +                              def->devnode); +        if (def->devlinks) { +            for (i = 0; def->devlinks[i]; i++) +                virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n", +                                      def->devlinks[i]); +        }       }       virBufferEscapeString(&buf, "<parent>%s</parent>\n", def->parent); -    if (def->driver) { +    if (def->driver && !inactive_state) {           virBufferAddLit(&buf, "<driver>\n");           virBufferAdjustIndent(&buf, 2);           virBufferEscapeString(&buf, "<name>%s</name>\n", def->driver); @@ -738,7 +747,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)               virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type));               break;           case VIR_NODE_DEV_CAP_MDEV: -            virNodeDeviceCapMdevDefFormat(&buf, data); +            virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state);               break;           case VIR_NODE_DEV_CAP_CCW_DEV:               virNodeDeviceCapCCWDefFormat(&buf, data); @@ -2226,7 +2235,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,       ctxt->node = node; -    if (!(mdev->dev_config.type = virXPathString("string(./type[1]/@id)", ctxt))) { +    if (!(mdev->defined_config.type = virXPathString("string(./type[1]/@id)", ctxt))) {           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,                          _("missing type id attribute for '%1$s'"), def->name);           return -1; @@ -2258,7 +2267,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,           return -1;       for (i = 0; i < nattrs; i++) -        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->dev_config); +        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->defined_config);       return 0;   } @@ -2601,11 +2610,15 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)           g_free(data->sg.path);           break;       case VIR_NODE_DEV_CAP_MDEV: -        g_free(data->mdev.dev_config.type); +        g_free(data->mdev.defined_config.type); +        g_free(data->mdev.active_config.type);           g_free(data->mdev.uuid); -        for (i = 0; i < data->mdev.dev_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.dev_config.attributes[i]); -        g_free(data->mdev.dev_config.attributes); +        for (i = 0; i < data->mdev.defined_config.nattributes; i++) + virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]); +        g_free(data->mdev.defined_config.attributes); +        for (i = 0; i < data->mdev.active_config.nattributes; i++) + virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]); +        g_free(data->mdev.active_config.attributes);           g_free(data->mdev.parent_addr);           break;       case VIR_NODE_DEV_CAP_CSS_DEV: diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index f100496272..f59440dbb9 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -153,7 +153,8 @@ typedef struct _virNodeDevCapMdev virNodeDevCapMdev;   struct _virNodeDevCapMdev {       unsigned int iommuGroupNumber;       char *uuid; -    virMediatedDeviceConfig dev_config; +    virMediatedDeviceConfig defined_config; +    virMediatedDeviceConfig active_config;       char *parent_addr;       bool autostart;   }; @@ -360,7 +361,7 @@ struct _virNodeDeviceDef {   };   char * -virNodeDeviceDefFormat(const virNodeDeviceDef *def); +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags);   typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef *dev, diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index f0f99bc020..9cc5c6466b 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -264,7 +264,7 @@ virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,   /**    * virNodeDeviceGetXMLDesc:    * @dev: pointer to the node device - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virNodeDeviceGetXMLDescFlags    *    * Fetch an XML document describing all aspects of    * the device. diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 0c8612eb71..d67c95d68d 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -338,7 +338,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,       virNodeDeviceDef *def;       char *ret = NULL; -    virCheckFlags(0, NULL); +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);       if (nodeDeviceInitWait() < 0)           return NULL; @@ -356,7 +356,19 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,       if (virNodeDeviceUpdateCaps(def) < 0)           goto cleanup; -    ret = virNodeDeviceDefFormat(def); +    if (flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) { +        if (!virNodeDeviceObjIsPersistent(obj)) { +            virReportError(VIR_ERR_OPERATION_INVALID, +                           _("node device '%1$s' is not persistent"), +                           def->name); +            goto cleanup; +        } +    } else { +        if (!virNodeDeviceObjIsActive(obj)) +            flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE; +    } + +    ret = virNodeDeviceDefFormat(def, flags);    cleanup:       virNodeDeviceObjEndAPI(&obj); @@ -629,19 +641,20 @@ nodeDeviceAttributesToJSON(virJSONValue *json,   /* format a json string that provides configuration information about this mdev    * to the mdevctl utility */   static int -nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf) +nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf, bool defined)

As far as I can tell, you never actually call this function with 'defined' set to false. It doesn't seem to be used in any of the following commits either.

Correct. I did not want to hardcode it in the method.

...

...

  {       virNodeDevCapMdev *mdev = &def->caps->data.mdev; +    virMediatedDeviceConfig mdev_config = defined ? mdev->defined_config : mdev->active_config;       g_autoptr(virJSONValue) json = virJSONValueNewObject();       const char *startval = mdev->autostart ? "auto" : "manual"; -    if (virJSONValueObjectAppendString(json, "mdev_type", mdev->dev_config.type) < 0) +    if (virJSONValueObjectAppendString(json, "mdev_type", mdev_config.type) < 0)           return -1;       if (virJSONValueObjectAppendString(json, "start", startval) < 0)           return -1; -    if (nodeDeviceAttributesToJSON(json, mdev->dev_config) < 0) +    if (nodeDeviceAttributesToJSON(json, mdev_config) < 0)           return -1;       *buf = virJSONValueToString(json, false); @@ -760,7 +773,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,               return NULL;           } -        if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) { +        if (nodeDeviceDefToMdevctlConfig(def, &inbuf, true) < 0) {               virReportError(VIR_ERR_INTERNAL_ERROR, "%s",                              _("couldn't convert node device def to mdevctl JSON"));               return NULL; @@ -1138,9 +1151,11 @@ nodeDeviceParseMdevctlAttribs(virMediatedDeviceConfig *config,   static virNodeDeviceDef*   nodeDeviceParseMdevctlChildDevice(const char *parent, -                                  virJSONValue *json) +                                  virJSONValue *json, +                                  bool defined)   {       virNodeDevCapMdev *mdev; +    virMediatedDeviceConfig *mdev_config;       const char *uuid;       virJSONValue *props;       g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); @@ -1170,14 +1185,15 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,       child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;       mdev = &child->caps->data.mdev; +    mdev_config = defined ? &mdev->defined_config : &mdev->active_config;       mdev->uuid = g_strdup(uuid);       mdev->parent_addr = g_strdup(parent); -    mdev->dev_config.type = +    mdev_config->type =           g_strdup(virJSONValueObjectGetString(props, "mdev_type"));       start = virJSONValueObjectGetString(props, "start");       mdev->autostart = STREQ_NULLABLE(start, "auto"); -    if (nodeDeviceParseMdevctlAttribs(&mdev->dev_config, +    if (nodeDeviceParseMdevctlAttribs(mdev_config,                                         virJSONValueObjectGet(props, "attrs")) < 0)           return NULL; @@ -1189,7 +1205,8 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,   int   nodeDeviceParseMdevctlJSON(const char *jsonstring, -                           virNodeDeviceDef ***devs) +                           virNodeDeviceDef ***devs, +                           bool defined)   {       int n;       g_autoptr(virJSONValue) json_devicelist = NULL; @@ -1259,7 +1276,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,               g_autoptr(virNodeDeviceDef) child = NULL;               virJSONValue *child_obj = virJSONValueArrayGet(child_array, j); -            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) { +            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj, defined))) {                   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",                                  _("Unable to parse child device"));                   goto error; @@ -1402,7 +1419,7 @@ nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def,           /* Active devices contain some additional information (e.g. sysfs            * path) that is not provided by mdevctl, so re-use the existing            * definition and copy over new mdev data */ -        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); +        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned, defined);           if (was_defined && !changed) {               /* if this device was already defined and the definition @@ -1672,7 +1689,7 @@ virMdevctlList(bool defined,           return -1;       } -    return nodeDeviceParseMdevctlJSON(output, devs); +    return nodeDeviceParseMdevctlJSON(output, devs, defined);   } @@ -1831,16 +1848,24 @@ virMediatedDeviceAttrsCopy(virMediatedDeviceConfig *dst_config,    * Returns true if anything was copied, else returns false */   bool   nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, -                             virNodeDeviceDef *src) +                             virNodeDeviceDef *src, +                             bool defined)   {       bool ret = false;       virNodeDevCapMdev *srcmdev = &src->caps->data.mdev;       virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev; +    virMediatedDeviceConfig *srcmdevconfig = &src->caps->data.mdev.active_config; +    virMediatedDeviceConfig *dstmdevconfig = &dst->caps->data.mdev.active_config; + +    if (defined) { +        srcmdevconfig = &src->caps->data.mdev.defined_config; +        dstmdevconfig = &dst->caps->data.mdev.defined_config; +    } -    if (STRNEQ_NULLABLE(dstmdev->dev_config.type, srcmdev->dev_config.type)) { +    if (STRNEQ_NULLABLE(dstmdevconfig->type, srcmdevconfig->type)) {           ret = true; -        g_free(dstmdev->dev_config.type); -        dstmdev->dev_config.type = g_strdup(srcmdev->dev_config.type); +        g_free(dstmdevconfig->type); +        dstmdevconfig->type = g_strdup(srcmdevconfig->type);       }       if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) { @@ -1849,7 +1874,7 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,           dstmdev->uuid = g_strdup(srcmdev->uuid);       } -    if (virMediatedDeviceAttrsCopy(&dstmdev->dev_config, &srcmdev->dev_config)) +    if (virMediatedDeviceAttrsCopy(dstmdevconfig, srcmdevconfig))           ret = true;       if (dstmdev->autostart != srcmdev->autostart) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index c7d5e22daf..4dce7e6f17 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -142,7 +142,8 @@ nodeDeviceGetMdevctlListCommand(bool defined,   int   nodeDeviceParseMdevctlJSON(const char *jsonstring, -                           virNodeDeviceDef ***devs); +                           virNodeDeviceDef ***devs, +                           bool defined);   int   nodeDeviceUpdateMediatedDevices(void); @@ -154,7 +155,8 @@ nodeDeviceGenerateName(virNodeDeviceDef *def,                          const char *s);   bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, -                                  virNodeDeviceDef *src); +                                  virNodeDeviceDef *src, +                                  bool defined);   int   nodeDeviceCreate(virNodeDevice *dev, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 254e802c50..57368a96c3 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1069,7 +1069,7 @@ udevProcessMediatedDevice(struct udev_device *dev,           return -1;       } -    data->dev_config.type = g_path_get_basename(canonicalpath); +    data->active_config.type = g_path_get_basename(canonicalpath);       data->uuid = g_strdup(udev_device_get_sysname(dev));       if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0) @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device)           objdef = virNodeDeviceObjGetDef(obj);           if (is_mdev) -            nodeDeviceDefCopyFromMdevctl(def, objdef); +            nodeDeviceDefCopyFromMdevctl(def, objdef, false);           persistent = virNodeDeviceObjIsPersistent(obj);           autostart = virNodeDeviceObjIsAutostart(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ed545848af..01863233bc 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7514,12 +7514,12 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,       virNodeDeviceObj *obj;       char *ret = NULL; -    virCheckFlags(0, NULL); +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);       if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))           return NULL; -    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj)); +    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);       virNodeDeviceObjEndAPI(&obj);       return ret; @@ -7619,7 +7619,7 @@ testNodeDeviceMockCreateVport(testDriver *driver,                                                      "scsi_host11")))           goto cleanup; -    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy)); +    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy), 0);       virNodeDeviceObjEndAPI(&objcopy);       if (!xml)           goto cleanup; diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index e403328e5a..852d9ed6e7 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -229,13 +229,13 @@ testMdevctlParse(const void *data)           return -1;       } -    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) { +    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs, true)) < 0) {           VIR_TEST_DEBUG("Unable to parse json for %s", filename);           return -1;       }       for (i = 0; i < nmdevs; i++) { -        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]); +        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i], VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE);           if (!devxml)               goto cleanup;           virBufferAddStr(&xmloutbuf, devxml); diff --git a/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml new file mode 100644 index 0000000000..6926559efa --- /dev/null +++ b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml @@ -0,0 +1,14 @@ +<device> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name> + <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path> +  <parent>css_0_0_0052</parent> +  <driver> +    <name>vfio_ccw_mdev</name> +  </driver> +  <capability type='mdev'> +    <type id='vfio_ccw-io'/> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid> +    <parent_addr>0.0.0052</parent_addr> +    <iommuGroup number='4'/> +  </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml new file mode 120000 index 0000000000..f8ec7d8a32 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml @@ -0,0 +1 @@ +mdev_3627463d_b7f0_4fea_b468_f1da537d301b.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml new file mode 100644 index 0000000000..82c60cc065 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml @@ -0,0 +1,10 @@ +<device> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name> + <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path> +  <parent>css_0_0_0052</parent> +  <capability type='mdev'> +    <type id='vfio_ccw-io'/> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid> +    <iommuGroup number='4'/> +  </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml new file mode 100644 index 0000000000..87c5ed1340 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml @@ -0,0 +1,9 @@ +<device> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name> +  <parent>css_0_0_0052</parent> +  <capability type='mdev'> +    <type id='vfio_ccw-io'/> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid> +    <iommuGroup number='4'/> +  </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml new file mode 120000 index 0000000000..f6d5789399 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml @@ -0,0 +1 @@ +mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml new file mode 120000 index 0000000000..233a7506fb --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml @@ -0,0 +1 @@ +mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml new file mode 100644 index 0000000000..8c4983f43c --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml @@ -0,0 +1,8 @@ +<device> +  <name>mdev_ee0b88c4-f554-4dc1-809d-b2a01e8e48ad</name> +  <parent>ap_matrix</parent> +  <capability type='mdev'> +    <type id='vfio_ap-passthrough'/> +    <iommuGroup number='0'/> +  </capability> +</device> diff --git a/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml new file mode 120000 index 0000000000..e053844177 --- /dev/null +++ b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml @@ -0,0 +1 @@ +mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml \ No newline at end of file diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 068ec68769..be7a5b4df9 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -11,14 +11,20 @@   #define VIR_FROM_THIS VIR_FROM_NONE +struct TestData { +    const char *filename; +    unsigned int flags; +}; +   static int -testCompareXMLToXMLFiles(const char *xml, const char *outfile) +testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flags)   {       g_autofree char *xmlData = NULL;       g_autofree char *actual = NULL;       int ret = -1;       virNodeDeviceDef *dev = NULL;       virNodeDevCapsDef *caps; +    size_t i;       if (virTestLoadFile(xml, &xmlData) < 0)           goto fail; @@ -46,9 +52,22 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile) data->storage.logical_block_size;               }           } + +        if (caps->data.type == VIR_NODE_DEV_CAP_MDEV && +            !(flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)) { +            data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type); +            for (i = 0; i < data->mdev.defined_config.nattributes; i++) { +                g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1); +                attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name); +                attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value); +                VIR_APPEND_ELEMENT(data->mdev.active_config.attributes, + data->mdev.active_config.nattributes, +                                   attr); +            } +        }       } -    if (!(actual = virNodeDeviceDefFormat(dev))) +    if (!(actual = virNodeDeviceDefFormat(dev, flags)))           goto fail;       if (virTestCompareToFile(actual, outfile) < 0) @@ -65,16 +84,21 @@ static int   testCompareXMLToXMLHelper(const void *data)   {       int result = -1; +    const struct TestData *tdata = data;       g_autofree char *xml = NULL;       g_autofree char *outfile = NULL;       xml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir, -                          (const char *)data); +                          tdata->filename); -    outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir, -                              (const char *)data); +    if (tdata->flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) +        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s_inactive.xml", abs_srcdir, +                                  tdata->filename); +    else +        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir, +                                  tdata->filename); -    result = testCompareXMLToXMLFiles(xml, outfile); +    result = testCompareXMLToXMLFiles(xml, outfile, tdata->flags);       return result;   } @@ -85,10 +109,20 @@ mymain(void)   {       int ret = 0; +#define DO_TEST_FLAGS(desc, filename, flags) \ +    do { \ +        struct TestData data = { filename, flags }; \ +        if (virTestRun(desc, testCompareXMLToXMLHelper, &data) < 0) \ +            ret = -1; \ +       } \ +    while (0) +   #define DO_TEST(name) \ -    if (virTestRun("Node device XML-2-XML " name, \ -                   testCompareXMLToXMLHelper, (name)) < 0) \ -        ret = -1 +    DO_TEST_FLAGS("Node device XML-2-XML " name, name, 0) + +#define DO_TEST_INACTIVE(name) \ +    DO_TEST_FLAGS("Node device XML-2-XML INACTIVE " name, \ +                  name, VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)       DO_TEST("computer");       DO_TEST("DVD_GCC_4247N"); @@ -121,6 +155,7 @@ mymain(void)       DO_TEST("pci_0000_02_10_7_mdev_types");       DO_TEST("pci_0000_42_00_0_vpd");       DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b"); +    DO_TEST_INACTIVE("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");       DO_TEST("ccw_0_0_ffff");       DO_TEST("css_0_0_ffff");       DO_TEST("css_0_0_ffff_channel_dev_addr"); @@ -134,7 +169,13 @@ mymain(void)       DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");       DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");       DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); +    DO_TEST_INACTIVE("mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad"); +    DO_TEST_INACTIVE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); +    DO_TEST_INACTIVE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); +    DO_TEST_INACTIVE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");       DO_TEST("hba_vport_ops"); +    DO_TEST("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c"); +    DO_TEST_INACTIVE("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");       return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;   }

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Thanks

...

_______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org

On 1/31/24 22:34, Jonathon Jongsma wrote:

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:

...

Allow to dump the XML of the persisted mdev when the mdev has been started instead of the current state only.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---   docs/manpages/virsh.rst |  7 +++++--   tools/virsh-nodedev.c   | 15 ++++++++++++++-   2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index ed1027e133..3a814dccd2 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5422,14 +5422,17 @@ nodedev-dumpxml   :: -   nodedev-dumpxml [--xpath EXPRESSION] [--wrap] device +   nodedev-dumpxml [--inactive] [--xpath EXPRESSION] [--wrap] device   Dump a <device> XML representation for the given node device, including   such information as the device name, which bus owns the device, the   vendor and product id, and any capabilities of the device usable by   libvirt (such as whether device reset is supported). *device* can   be either device name or wwn pair in "wwnn,wwpn" format (only works -for HBA). +for HBA). An additional option affecting the XML dump may be +used. *--inactive* tells virsh to dump the node device configuration +that will be used on next start of the node device as opposed to the +current node device configuration.   If the **--xpath** argument provides an XPath expression, it will be   evaluated against the output XML and only those matching nodes will diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index fb38fd7fcc..54e192d619 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -575,6 +575,10 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = {        .help = N_("device name or wwn pair in 'wwnn,wwpn' format"),        .completer = virshNodeDeviceNameCompleter,       }, +    {.name = "inactive", +     .type = VSH_OT_BOOL, +     .help = N_("show inactive defined XML"), +    },       {.name = "xpath",        .type = VSH_OT_STRING,        .flags = VSH_OFLAG_REQ_OPT, @@ -594,6 +598,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)       g_autoptr(virshNodeDevice) device = NULL;       g_autofree char *xml = NULL;       const char *device_value = NULL; +    unsigned int flags = 0;       bool wrap = vshCommandOptBool(cmd, "wrap");       const char *xpath = NULL; @@ -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?

...

+    } + +    if (!(xml = virNodeDeviceGetXMLDesc(device, flags)))           return false;       return virshDumpXML(ctl, xml, "node-device", xpath, wrap);

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Thanks

_______________________________________________ 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

On 2/1/24 17:39, Jonathon Jongsma wrote:

...

...

...

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

Ok, I removed the precheck. -- 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

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:

Allow to filter node devices based on their persisted or transient states.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- include/libvirt/libvirt-nodedev.h | 2 ++ src/conf/node_device_conf.h | 7 ++++++- src/conf/virnodedeviceobj.c | 8 ++++++++ 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index d18246e2f6..dff394ec86 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -91,6 +91,8 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX = 1 << 20, /* s390 AP Matrix (Since: 7.0.0) */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPD = 1 << 21, /* Device with VPD (Since: 7.9.0) */

+ VIR_CONNECT_LIST_NODE_DEVICES_PERSISTED = 1 << 28, /* Persisted devices (Since: 10.1.0) */

s/PERSISTED/PERSISTENT/

+ VIR_CONNECT_LIST_NODE_DEVICES_TRANSIENT = 1 << 29, /* Transient devices (Since: 10.1.0) */ VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE = 1 << 30, /* Inactive devices (Since: 7.3.0) */ VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE = 1U << 31, /* Active devices (Since: 7.3.0) */ } virConnectListAllNodeDeviceFlags; diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index f59440dbb9..3ee1fbc665 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -432,9 +432,14 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDevCapsDef, virNodeDevCapsDefFree); VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE | \ VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE

+#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_PERSIST \

s/PERSIST/PERSISTENT/

+ VIR_CONNECT_LIST_NODE_DEVICES_PERSISTED | \

s/PERSISTED/PERSISTENT/

+ VIR_CONNECT_LIST_NODE_DEVICES_TRANSIENT + #define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL \ VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP | \ - VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE + VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE | \ + VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_PERSIST

int virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHost *scsi_host); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index cfef30d47e..0b9ca8c864 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -911,6 +911,14 @@ virNodeDeviceObjMatch(virNodeDeviceObj *obj, return false; }

+ if (flags & (VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_PERSIST)) { + if (!((MATCH(VIR_CONNECT_LIST_NODE_DEVICES_PERSISTED) && + virNodeDeviceObjIsPersistent(obj)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_TRANSIENT) && + !virNodeDeviceObjIsPersistent(obj)))) + return false; + } + return true; } #undef MATCH

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

On 1/31/24 22:38, Jonathon Jongsma wrote:

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:

...

Now that we can filter persisted and transient node devices in virConnectListAllNodeDevices(), add these switches also to the virsh nodedev-list command.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---   docs/manpages/virsh.rst |  8 ++++++--   tools/virsh-nodedev.c   | 24 ++++++++++++++++++++++--   2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 3a814dccd2..4bcfbc230b 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5461,7 +5461,7 @@ nodedev-list   :: -   nodedev-list [--cap capability] [--tree] [--inactive | --all] +   nodedev-list [--cap capability] [--tree] [--inactive | --all] [--persisted | --transient]

s/persisted/persistent/

...

  List all of the devices available on the node that are known by libvirt.   *cap* is used to filter the list by capability types, the types must be @@ -5470,7 +5470,11 @@ separated by comma, e.g. --cap pci,scsi. Valid capability types include   'scsi', 'storage', 'fc_host', 'vports', 'scsi_generic', 'drm', 'mdev',   'mdev_types', 'ccw', 'css', 'ap_card', 'ap_queue', 'ap_matrix'. By default,   only active devices are listed. *--inactive* is used to list only inactive -devices, and *-all* is used to list both active and inactive devices. +devices, and *--all* is used to list both active and inactive devices. +*--persisted* is used to list only persisted devices, and *--transient* is

here too

...

+used to list only transient devices. Not providing *--persisted* or +*--transient* will list all devices unless filtered otherwise. *--transient* +is mutually exclusive with *--persisted* and *--inactive*.

again

...

  If *--tree* is used, the output is formatted in a tree representing parents of   each node.  *--tree* is mutually exclusive with all other options. diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 54e192d619..4acb955efe 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -390,6 +390,14 @@ static const vshCmdOptDef opts_node_list_devices[] = {        .type = VSH_OT_BOOL,        .help = N_("list inactive & active devices")       }, +    {.name = "persisted",

again

...

+     .type = VSH_OT_BOOL, +     .help = N_("list persisted devices")

again

...

+    }, +    {.name = "transient", +     .type = VSH_OT_BOOL, +     .help = N_("list transient devices") +    },       {.name = NULL}   }; @@ -407,6 +415,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED)       int cap_type = -1;       bool inactive = vshCommandOptBool(cmd, "inactive");       bool all = vshCommandOptBool(cmd, "all"); +    bool persisted = vshCommandOptBool(cmd, "persisted");

again

...

+    bool transient = vshCommandOptBool(cmd, "transient");       ignore_value(vshCommandOptStringQuiet(ctl, cmd, "cap", &cap_str)); @@ -420,8 +430,13 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED)           return false;       } -    if (tree && (cap_str || inactive)) { -        vshError(ctl, "%s", _("Option --tree is incompatible with --cap and --inactive")); +    if (transient && (persisted || inactive)) { +        vshError(ctl, "%s", _("Option --transient is incompatible with --persisted and --inactive"));

again

...

+        return false; +    } + +    if (tree && (cap_str || inactive || persisted || transient)) { +        vshError(ctl, "%s", _("Option --tree is incompatible with --cap, --inactive, --persisted and --transient"));

again

All changed

...

          return false;       } @@ -509,6 +524,11 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED)       if (!inactive)           flags |= VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE; +    if (persisted) +        flags |= VIR_CONNECT_LIST_NODE_DEVICES_PERSISTED; +    if (transient) +        flags |= VIR_CONNECT_LIST_NODE_DEVICES_TRANSIENT; +       if (!(list = virshNodeDeviceListCollect(ctl, caps, ncaps, flags))) {           ret = false;           goto cleanup;

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Thanks

_______________________________________________ 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

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:

Implement the API functions in the node device driver by using mdevctl modify with the options defined and live. Increase the minimum mdevctl version to 1.3.0 in spec file to ensure support exists in mdevctl.

mdevctl 1.3.0 was only released about 3 weeks ago, so I don't think we can add an unconditional dependency on it. I think we'll have to make sure that we can degrade gracefully when the required version isn't available.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- libvirt.spec.in | 2 +- src/node_device/node_device_driver.c | 181 ++++++++++++++++++- src/node_device/node_device_driver.h | 11 ++ src/node_device/node_device_udev.c | 1 + tests/nodedevmdevctldata/mdevctl-modify.argv | 19 ++ tests/nodedevmdevctltest.c | 64 +++++++ 6 files changed, 275 insertions(+), 3 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-modify.argv

diff --git a/libvirt.spec.in b/libvirt.spec.in index 8413e3c19a..3ed14fa63c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -613,7 +613,7 @@ Requires: libvirt-libs = %{version}-%{release} # needed for device enumeration Requires: systemd >= 185 # For managing persistent mediated devices -Requires: mdevctl +Requires: mdevctl >= 1.3.0 # for modprobe of pci devices Requires: module-init-tools

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d67c95d68d..dd57e9ca5b 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -54,7 +54,7 @@ virNodeDeviceDriverState *driver;

VIR_ENUM_IMPL(virMdevctlCommand, MDEVCTL_CMD_LAST, - "start", "stop", "define", "undefine", "create" + "start", "stop", "define", "undefine", "create", "modify" );

@@ -754,6 +754,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, case MDEVCTL_CMD_START: case MDEVCTL_CMD_DEFINE: case MDEVCTL_CMD_UNDEFINE: + case MDEVCTL_CMD_MODIFY: cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL); break; case MDEVCTL_CMD_LAST: @@ -767,6 +768,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, switch (cmd_type) { case MDEVCTL_CMD_CREATE: case MDEVCTL_CMD_DEFINE: + case MDEVCTL_CMD_MODIFY: if (!def->caps->data.mdev.parent_addr) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to find parent device '%1$s'"), def->parent); @@ -783,7 +785,8 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");

virCommandSetInputBuffer(cmd, inbuf); - virCommandSetOutputBuffer(cmd, outbuf); + if (outbuf) + virCommandSetOutputBuffer(cmd, outbuf); break;

case MDEVCTL_CMD_UNDEFINE: @@ -868,6 +871,61 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid) }

+/* gets a virCommand object that executes a mdevctl command to modify a + * a device to an updated version + */ +virCommand* +nodeDeviceGetMdevctlModifyCommand(virNodeDeviceDef *def, + bool defined, + bool live, + char **errmsg) +{ + virCommand *cmd = nodeDeviceGetMdevctlCommand(def, + MDEVCTL_CMD_MODIFY, + NULL, errmsg); + + if (!cmd) + return NULL; + + if (defined) + virCommandAddArg(cmd, "--defined"); + + if (live) + virCommandAddArg(cmd, "--live"); + + return cmd; +} + + +static int +virMdevctlModify(virNodeDeviceDef *def, + bool defined, + bool live) +{ + int status; + g_autofree char *errmsg = NULL; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlModifyCommand(def, + defined, + live, + &errmsg); + + if (!cmd) + return -1; + + if (virCommandRun(cmd, &status) < 0) + return -1; + + if (status != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to modify mediated device: %1$s"), + MDEVCTL_ERROR(errmsg)); + return -1; + } + + return 0; +} + + static virNodeDevicePtr nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDef *def) @@ -2108,3 +2166,122 @@ nodeDeviceIsActive(virNodeDevice *device) virNodeDeviceObjEndAPI(&obj); return ret; } + + +static int +nodeDeviceDefCompareMdevs(virNodeDeviceDef *def_cur, + virNodeDeviceDef *def_upd, + bool live) +{ + virNodeDevCapsDef *caps = NULL; + virNodeDevCapMdev *cap_mdev_cur = NULL; + virNodeDevCapMdev *cap_mdev_upd = NULL; + + for (caps = def_cur->caps; caps != NULL; caps = caps->next) { + if (caps->data.type == VIR_NODE_DEV_CAP_MDEV) { + cap_mdev_cur = &caps->data.mdev; + } + } + if (!cap_mdev_cur) + return -1; + + for (caps = def_upd->caps; caps != NULL; caps = caps->next) { + if (caps->data.type == VIR_NODE_DEV_CAP_MDEV) { + cap_mdev_upd = &caps->data.mdev; + } + } + if (!cap_mdev_upd) + return -1; + + if (STRNEQ_NULLABLE(cap_mdev_cur->uuid, cap_mdev_upd->uuid)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("uuid mismatch (mdev:'%1$s' update:'%2$s')"),

I'd suggest a more complete description of the error so the context is a bit more clear when scanning the log. Something like: "Cannot update device $NAME, uuid mismatch". I'm not sure if we need to log the actual values.

+ cap_mdev_cur->uuid, + cap_mdev_upd->uuid); + return -1; + } + // for a live update the types of the active configs must match! + if (live && + STRNEQ_NULLABLE(cap_mdev_cur->active_config.type, cap_mdev_upd->active_config.type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("type mismatch (mdev:'%1$s' update:'%2$s')"),

same

+ cap_mdev_cur->active_config.type, + cap_mdev_upd->active_config.type); + return -1; + } + if (STRNEQ_NULLABLE(cap_mdev_cur->parent_addr, cap_mdev_upd->parent_addr)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("parent address mismatch (mdev:'%1$s' update:'%2$s')"),

same

+ cap_mdev_cur->parent_addr, + cap_mdev_upd->parent_addr); + return -1; + } + + return 0; +} + + +int +nodeDeviceUpdateXML(virNodeDevice *device, + const char *xmlDesc, + unsigned int flags) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def_cur; + g_autoptr(virNodeDeviceDef) def_upd = NULL; + const char *virt_type = NULL; + bool updated = false; + + virCheckFlags(VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE | + VIR_NODE_DEVICE_UPDATE_XML_AFFECT_CONFIG, -1); + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def_cur = virNodeDeviceObjGetDef(obj); + + virt_type = virConnectGetType(device->conn); + + if (virNodeDeviceUpdateXMLEnsureACL(device->conn, def_cur, flags) < 0) + goto cleanup; + + if (!(def_upd = virNodeDeviceDefParse(xmlDesc, NULL, EXISTING_DEVICE, virt_type, + &driver->parserCallbacks, NULL, true))) + goto cleanup; + + if (nodeDeviceHasCapability(def_cur, VIR_NODE_DEV_CAP_MDEV) && + nodeDeviceHasCapability(def_upd, VIR_NODE_DEV_CAP_MDEV)) { + // Checks flags are valid for the state and sets flags for current if flags not set + if (virNodeDeviceObjUpdateModificationImpact(obj, &flags) < 0) + goto cleanup; + + // Compare def_cur and def_upd for compatibleness e.g. parent, type and uuid + if (nodeDeviceDefCompareMdevs(def_cur, def_upd, + (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE)) < 0) + goto cleanup; + + // Update now + VIR_DEBUG("Update node device '%s' with mdevctl", def_cur->name); + if (virMdevctlModify(def_upd, + (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_CONFIG), + (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE)) < 0) { + goto cleanup; + }; + // Detect updates and also trigger events + updated = true; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type"));

you need to goto cleanup here otherwise the return value will be set to 0.

+ } + + ret = 0; + cleanup: + virNodeDeviceObjEndAPI(&obj); + if (updated) + nodeDeviceUpdateMediatedDevices(); + + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 4dce7e6f17..5751625eda 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -39,6 +39,7 @@ typedef enum { * separation makes our code more readable in terms of knowing when we're * starting a defined device and when we're creating a transient one */ MDEVCTL_CMD_CREATE, + MDEVCTL_CMD_MODIFY,

MDEVCTL_CMD_LAST, } virMdevctlCommand; @@ -186,3 +187,13 @@ virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, char **errmsg); + +virCommand* +nodeDeviceGetMdevctlModifyCommand(virNodeDeviceDef *def, + bool defined, + bool live, + char **errmsg); +int +nodeDeviceUpdateXML(virNodeDevice *dev, + const char *xmlDesc, + unsigned int flags); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 57368a96c3..4cd9d285fa 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2403,6 +2403,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.8.0 */ .nodeDeviceIsPersistent = nodeDeviceIsPersistent, /* 7.8.0 */ .nodeDeviceIsActive = nodeDeviceIsActive, /* 7.8.0 */ + .nodeDeviceUpdateXML = nodeDeviceUpdateXML, /* 10.1.0 */ };

diff --git a/tests/nodedevmdevctldata/mdevctl-modify.argv b/tests/nodedevmdevctldata/mdevctl-modify.argv new file mode 100644 index 0000000000..6acb9ca0bf --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-modify.argv @@ -0,0 +1,19 @@ +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--defined +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--live +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--defined \ +--live diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 852d9ed6e7..37978c9a5c 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -63,6 +63,7 @@ testMdevctlCmd(virMdevctlCommand cmd_type, case MDEVCTL_CMD_START: case MDEVCTL_CMD_STOP: case MDEVCTL_CMD_UNDEFINE: + case MDEVCTL_CMD_MODIFY: create = EXISTING_DEVICE; break; case MDEVCTL_CMD_LAST: @@ -173,6 +174,64 @@ testMdevctlAutostart(const void *data G_GNUC_UNUSED) return ret; }

+ +static int +testMdevctlModify(const void *data G_GNUC_UNUSED) +{ + g_autoptr(virNodeDeviceDef) def = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autoptr(virCommand) definedcmd = NULL; + g_autoptr(virCommand) livecmd = NULL; + g_autoptr(virCommand) bothcmd = NULL; + g_autofree char *errmsg = NULL; + /* just concatenate both calls into the same output file */ + g_autofree char *cmdlinefile = + g_strdup_printf("%s/nodedevmdevctldata/mdevctl-modify.argv", + abs_srcdir); + g_autofree char *mdevxml = + g_strdup_printf("%s/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml", + abs_srcdir); + g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); + + if (!(def = virNodeDeviceDefParse(NULL, mdevxml, CREATE_DEVICE, VIRT_TYPE, + &parser_callbacks, NULL, false))) + return -1; + + virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL);

You should also capture the stdin since that's really the majority of the command input.

+ + if (!(definedcmd = nodeDeviceGetMdevctlModifyCommand(def, true, false, &errmsg))) + goto cleanup; + + if (virCommandRun(definedcmd, NULL) < 0) + goto cleanup; + + if (!(livecmd = nodeDeviceGetMdevctlModifyCommand(def, false, true, &errmsg))) + goto cleanup; + + if (virCommandRun(livecmd, NULL) < 0) + goto cleanup; + + if (!(bothcmd = nodeDeviceGetMdevctlModifyCommand(def, true, true, &errmsg))) + goto cleanup; + + if (virCommandRun(bothcmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; + + if (virTestCompareToFileFull(actualCmdline, cmdlinefile, false) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + static int testMdevctlListDefined(const void *data G_GNUC_UNUSED) { @@ -457,6 +516,9 @@ mymain(void) #define DO_TEST_AUTOSTART() \ DO_TEST_FULL("autostart mdevs", testMdevctlAutostart, NULL)

+#define DO_TEST_MODIFY() \ + DO_TEST_FULL("modify mdevs", testMdevctlModify, NULL) + #define DO_TEST_PARSE_JSON(filename) \ DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename)

@@ -485,6 +547,8 @@ mymain(void)

DO_TEST_AUTOSTART();

+ DO_TEST_MODIFY(); + done: nodedevTestDriverFree(driver);

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com

On 2/1/24 11:07 AM, Boris Fiuczynski wrote:

On 1/31/24 22:41, Jonathon Jongsma wrote:

...

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:

...

Implement the API functions in the node device driver by using mdevctl modify with the options defined and live. Increase the minimum mdevctl version to 1.3.0 in spec file to ensure support exists in mdevctl.

mdevctl 1.3.0 was only released about 3 weeks ago, so I don't think we can add an unconditional dependency on it. I think we'll have to make sure that we can degrade gracefully when the required version isn't available.

What is your suggestion? 1) Adding a mdevctl version check to the runtime code, which in general can cause trouble interpreting the verion number with regard to distro backports. 2) Before really calling modify find out if mdevctl supports it by issuing e.g. "mdevctl modify --live --help". If live is support the RC is 0 if it is not supported the RC is 2. 3) Something else?

Hmm, I was thinking that we only needed the 1.3.0 version for the --live and --defined options, but I now realize that we also need the --jsonfile argument from 1.3.0. That makes it a bit harder to degrade gracefully. However, even if 1.3.0 is installed, not all devices will be able to be live-updated. So we need to handle errors in command execution regardless. If the user has an older mdevctl, the command will return an error and the user will be unable to update an mdev via the new API, which is the same behavior as they had before this patch. If the user has a newer mdevctl, it will just work. Maybe that's good enough for now. Jonathon

...

...

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---   libvirt.spec.in                              |   2 +-   src/node_device/node_device_driver.c         | 181 ++++++++++++++++++-   src/node_device/node_device_driver.h         |  11 ++   src/node_device/node_device_udev.c           |   1 +   tests/nodedevmdevctldata/mdevctl-modify.argv |  19 ++   tests/nodedevmdevctltest.c                   |  64 +++++++   6 files changed, 275 insertions(+), 3 deletions(-)   create mode 100644 tests/nodedevmdevctldata/mdevctl-modify.argv

diff --git a/libvirt.spec.in b/libvirt.spec.in index 8413e3c19a..3ed14fa63c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -613,7 +613,7 @@ Requires: libvirt-libs = %{version}-%{release}   # needed for device enumeration   Requires: systemd >= 185   # For managing persistent mediated devices -Requires: mdevctl +Requires: mdevctl >= 1.3.0   # for modprobe of pci devices   Requires: module-init-tools diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d67c95d68d..dd57e9ca5b 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -54,7 +54,7 @@ virNodeDeviceDriverState *driver;   VIR_ENUM_IMPL(virMdevctlCommand,                 MDEVCTL_CMD_LAST, -              "start", "stop", "define", "undefine", "create" +              "start", "stop", "define", "undefine", "create", "modify"   ); @@ -754,6 +754,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,       case MDEVCTL_CMD_START:       case MDEVCTL_CMD_DEFINE:       case MDEVCTL_CMD_UNDEFINE: +    case MDEVCTL_CMD_MODIFY:           cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);           break;       case MDEVCTL_CMD_LAST: @@ -767,6 +768,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,       switch (cmd_type) {       case MDEVCTL_CMD_CREATE:       case MDEVCTL_CMD_DEFINE: +    case MDEVCTL_CMD_MODIFY:           if (!def->caps->data.mdev.parent_addr) {               virReportError(VIR_ERR_INTERNAL_ERROR,                              _("unable to find parent device '%1$s'"), def->parent); @@ -783,7 +785,8 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,           virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");           virCommandSetInputBuffer(cmd, inbuf); -        virCommandSetOutputBuffer(cmd, outbuf); +        if (outbuf) +            virCommandSetOutputBuffer(cmd, outbuf);           break;       case MDEVCTL_CMD_UNDEFINE: @@ -868,6 +871,61 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid)   } +/* gets a virCommand object that executes a mdevctl command to modify a + * a device to an updated version + */ +virCommand* +nodeDeviceGetMdevctlModifyCommand(virNodeDeviceDef *def, +                                  bool defined, +                                  bool live, +                                  char **errmsg) +{ +    virCommand *cmd = nodeDeviceGetMdevctlCommand(def, +                                                  MDEVCTL_CMD_MODIFY, +                                                  NULL, errmsg); + +    if (!cmd) +        return NULL; + +    if (defined) +        virCommandAddArg(cmd, "--defined"); + +    if (live) +        virCommandAddArg(cmd, "--live"); + +    return cmd; +} + + +static int +virMdevctlModify(virNodeDeviceDef *def, +                 bool defined, +                 bool live) +{ +    int status; +    g_autofree char *errmsg = NULL; +    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlModifyCommand(def, + defined, +                                                                  live, + &errmsg); + +    if (!cmd) +        return -1; + +    if (virCommandRun(cmd, &status) < 0) +        return -1; + +    if (status != 0) { +        virReportError(VIR_ERR_INTERNAL_ERROR, +                       _("Unable to modify mediated device: %1$s"), +                       MDEVCTL_ERROR(errmsg)); +        return -1; +    } + +    return 0; +} + +   static virNodeDevicePtr   nodeDeviceCreateXMLMdev(virConnectPtr conn,                           virNodeDeviceDef *def) @@ -2108,3 +2166,122 @@ nodeDeviceIsActive(virNodeDevice *device)       virNodeDeviceObjEndAPI(&obj);       return ret;   } + + +static int +nodeDeviceDefCompareMdevs(virNodeDeviceDef *def_cur, +                          virNodeDeviceDef *def_upd, +                          bool live) +{ +    virNodeDevCapsDef *caps = NULL; +    virNodeDevCapMdev *cap_mdev_cur = NULL; +    virNodeDevCapMdev *cap_mdev_upd = NULL; + +    for (caps = def_cur->caps; caps != NULL; caps = caps->next) { +        if (caps->data.type == VIR_NODE_DEV_CAP_MDEV) { +            cap_mdev_cur = &caps->data.mdev; +        } +    } +    if (!cap_mdev_cur) +        return -1; + +    for (caps = def_upd->caps; caps != NULL; caps = caps->next) { +        if (caps->data.type == VIR_NODE_DEV_CAP_MDEV) { +            cap_mdev_upd = &caps->data.mdev; +        } +    } +    if (!cap_mdev_upd) +        return -1; + +    if (STRNEQ_NULLABLE(cap_mdev_cur->uuid, cap_mdev_upd->uuid)) { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, +                       _("uuid mismatch (mdev:'%1$s' update:'%2$s')"),

I'd suggest a more complete description of the error so the context is a bit more clear when scanning the log. Something like: "Cannot update device $NAME, uuid mismatch". I'm not sure if we need to log the actual values.

...

+                       cap_mdev_cur->uuid, +                       cap_mdev_upd->uuid); +        return -1; +    } +    // for a live update the types of the active configs must match! +    if (live && +        STRNEQ_NULLABLE(cap_mdev_cur->active_config.type, cap_mdev_upd->active_config.type)) { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, +                       _("type mismatch (mdev:'%1$s' update:'%2$s')"),

same

...

+                       cap_mdev_cur->active_config.type, +                       cap_mdev_upd->active_config.type); +        return -1; +    } +    if (STRNEQ_NULLABLE(cap_mdev_cur->parent_addr, cap_mdev_upd->parent_addr)) { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, +                       _("parent address mismatch (mdev:'%1$s' update:'%2$s')"),

same

Changed the message as you suggested and added at the end the current value so that someone can change it in the XML used for the update. E.g.         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,                        _("Cannot update device '%1$s', parent address mismatch (current parent address '%2$s')"),                        dev_cur->name,                        cap_mdev_cur->parent_addr);

...

...

+                       cap_mdev_cur->parent_addr, +                       cap_mdev_upd->parent_addr); +        return -1; +    } + +    return 0; +} + + +int +nodeDeviceUpdateXML(virNodeDevice *device, +                    const char *xmlDesc, +                    unsigned int flags) +{ +    int ret = -1; +    virNodeDeviceObj *obj = NULL; +    virNodeDeviceDef *def_cur; +    g_autoptr(virNodeDeviceDef) def_upd = NULL; +    const char *virt_type = NULL; +    bool updated = false; + +    virCheckFlags(VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE | +                  VIR_NODE_DEVICE_UPDATE_XML_AFFECT_CONFIG, -1); + +    if (nodeDeviceInitWait() < 0) +        return -1; + +    if (!(obj = nodeDeviceObjFindByName(device->name))) +        return -1; +    def_cur = virNodeDeviceObjGetDef(obj); + +    virt_type = virConnectGetType(device->conn); + +    if (virNodeDeviceUpdateXMLEnsureACL(device->conn, def_cur, flags) < 0) +        goto cleanup; + +    if (!(def_upd = virNodeDeviceDefParse(xmlDesc, NULL, EXISTING_DEVICE, virt_type, +                                          &driver->parserCallbacks, NULL, true))) +        goto cleanup; + +    if (nodeDeviceHasCapability(def_cur, VIR_NODE_DEV_CAP_MDEV) && +        nodeDeviceHasCapability(def_upd, VIR_NODE_DEV_CAP_MDEV)) { +        // Checks flags are valid for the state and sets flags for current if flags not set +        if (virNodeDeviceObjUpdateModificationImpact(obj, &flags) < 0) +            goto cleanup; + +        // Compare def_cur and def_upd for compatibleness e.g. parent, type and uuid +        if (nodeDeviceDefCompareMdevs(def_cur, def_upd, +                                      (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE)) < 0) +            goto cleanup; + +        // Update now +        VIR_DEBUG("Update node device '%s' with mdevctl", def_cur->name); +        if (virMdevctlModify(def_upd, +                             (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_CONFIG), +                             (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE)) < 0) { +            goto cleanup; +        }; +        // Detect updates and also trigger events +        updated = true; +    } else { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", +                       _("Unsupported device type"));

you need to goto cleanup here otherwise the return value will be set to 0.

Ups, certainly correct.

...

...

+    } + +    ret = 0; + cleanup: +    virNodeDeviceObjEndAPI(&obj); +    if (updated) +        nodeDeviceUpdateMediatedDevices(); + +    return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 4dce7e6f17..5751625eda 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -39,6 +39,7 @@ typedef enum {        * separation makes our code more readable in terms of knowing when we're        * starting a defined device and when we're creating a transient one */       MDEVCTL_CMD_CREATE, +    MDEVCTL_CMD_MODIFY,       MDEVCTL_CMD_LAST,   } virMdevctlCommand; @@ -186,3 +187,13 @@ virCommand*   nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def,                                           bool autostart,                                           char **errmsg); + +virCommand* +nodeDeviceGetMdevctlModifyCommand(virNodeDeviceDef *def, +                                  bool defined, +                                  bool live, +                                  char **errmsg); +int +nodeDeviceUpdateXML(virNodeDevice *dev, +                    const char *xmlDesc, +                    unsigned int flags); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 57368a96c3..4cd9d285fa 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2403,6 +2403,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = {       .nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.8.0 */       .nodeDeviceIsPersistent = nodeDeviceIsPersistent, /* 7.8.0 */       .nodeDeviceIsActive = nodeDeviceIsActive, /* 7.8.0 */ +    .nodeDeviceUpdateXML = nodeDeviceUpdateXML, /* 10.1.0 */   }; diff --git a/tests/nodedevmdevctldata/mdevctl-modify.argv b/tests/nodedevmdevctldata/mdevctl-modify.argv new file mode 100644 index 0000000000..6acb9ca0bf --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-modify.argv @@ -0,0 +1,19 @@ +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--defined +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--live +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--defined \ +--live diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 852d9ed6e7..37978c9a5c 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -63,6 +63,7 @@ testMdevctlCmd(virMdevctlCommand cmd_type,           case MDEVCTL_CMD_START:           case MDEVCTL_CMD_STOP:           case MDEVCTL_CMD_UNDEFINE: +        case MDEVCTL_CMD_MODIFY:               create = EXISTING_DEVICE;               break;           case MDEVCTL_CMD_LAST: @@ -173,6 +174,64 @@ testMdevctlAutostart(const void *data G_GNUC_UNUSED)       return ret;   } + +static int +testMdevctlModify(const void *data G_GNUC_UNUSED) +{ +    g_autoptr(virNodeDeviceDef) def = NULL; +    virBuffer buf = VIR_BUFFER_INITIALIZER; +    const char *actualCmdline = NULL; +    int ret = -1; +    g_autoptr(virCommand) definedcmd = NULL; +    g_autoptr(virCommand) livecmd = NULL; +    g_autoptr(virCommand) bothcmd = NULL; +    g_autofree char *errmsg = NULL; +    /* just concatenate both calls into the same output file */ +    g_autofree char *cmdlinefile = +        g_strdup_printf("%s/nodedevmdevctldata/mdevctl-modify.argv", +                        abs_srcdir); +    g_autofree char *mdevxml = + g_strdup_printf("%s/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml", +                        abs_srcdir); +    g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); + +    if (!(def = virNodeDeviceDefParse(NULL, mdevxml, CREATE_DEVICE, VIRT_TYPE, +                                      &parser_callbacks, NULL, false))) +        return -1; + +    virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL);

You should also capture the stdin since that's really the majority of the command input.

Ok, I agree and added the stdin check.

...

...

+ +    if (!(definedcmd = nodeDeviceGetMdevctlModifyCommand(def, true, false, &errmsg))) +        goto cleanup; + +    if (virCommandRun(definedcmd, NULL) < 0) +        goto cleanup; + +    if (!(livecmd = nodeDeviceGetMdevctlModifyCommand(def, false, true, &errmsg))) +        goto cleanup; + +    if (virCommandRun(livecmd, NULL) < 0) +        goto cleanup; + +    if (!(bothcmd = nodeDeviceGetMdevctlModifyCommand(def, true, true, &errmsg))) +        goto cleanup; + +    if (virCommandRun(bothcmd, NULL) < 0) +        goto cleanup; + +    if (!(actualCmdline = virBufferCurrentContent(&buf))) +        goto cleanup; + +    if (virTestCompareToFileFull(actualCmdline, cmdlinefile, false) < 0) +        goto cleanup; + +    ret = 0; + + cleanup: +    virBufferFreeAndReset(&buf); +    return ret; +} +   static int   testMdevctlListDefined(const void *data G_GNUC_UNUSED)   { @@ -457,6 +516,9 @@ mymain(void)   #define DO_TEST_AUTOSTART() \       DO_TEST_FULL("autostart mdevs", testMdevctlAutostart, NULL) +#define DO_TEST_MODIFY() \ +    DO_TEST_FULL("modify mdevs", testMdevctlModify, NULL) +   #define DO_TEST_PARSE_JSON(filename) \       DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) @@ -485,6 +547,8 @@ mymain(void)       DO_TEST_AUTOSTART(); +    DO_TEST_MODIFY(); +    done:       nodedevTestDriverFree(driver);

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com

Thanks

...

_______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org