9:28 a.m.
On 2/14/24 20:07, Jonathon Jongsma wrote:
On 2/13/24 3:04 AM, Boris Fiuczynski wrote:
> On 2/9/24 23:45, Jonathon Jongsma wrote:
>> On 2/7/24 7:39 AM, Boris Fiuczynski wrote:
>>> Implement the API functions in the node device driver by using
>>> mdevctl modify with the options defined and live.
>>> Instead of increasing the minimum mdevctl version to 1.3.0 in spec file
>>> to ensure support exists in mdevctl the support is dynamically checked
>>> before using mdevctl.
>>>
>>> Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>>> ---
>>> NEWS.rst | 7 +
>>> libvirt.spec.in | 1 +
>>> src/node_device/node_device_driver.c | 256
>>> +++++++++++++++++-
>>> src/node_device/node_device_driver.h | 11 +
>>> src/node_device/node_device_udev.c | 1 +
>>> ...60c_c60c_c60c_c60c_c60cc60cc60c_update.xml | 16 ++
>>> tests/nodedevmdevctldata/mdevctl-modify.argv | 25 ++
>>> tests/nodedevmdevctldata/mdevctl-modify.json | 1 +
>>> tests/nodedevmdevctltest.c | 90 +++++-
>>> 9 files changed, 405 insertions(+), 3 deletions(-)
>>> create mode 100644
>>>
tests/nodedevmdevctldata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_update.xml
>>> create mode 100644 tests/nodedevmdevctldata/mdevctl-modify.argv
>>> create mode 100644 tests/nodedevmdevctldata/mdevctl-modify.json
>>>
>>> diff --git a/NEWS.rst b/NEWS.rst
>>> index e2796fd8b2..aa2fee3533 100644
>>> --- a/NEWS.rst
>>> +++ b/NEWS.rst
>>> @@ -17,6 +17,13 @@ v10.1.0 (unreleased)
>>> * **New features**
>>> + * nodedev: Support updating mdevs
>>> +
>>> + The node device driver has been extended to allow updating
>>> mediated node
>>> + devices. Options are available to target the update against the
>>> persisted,
>>> + active or both configurations of an mediated device.
>>> + **Note:** The support is only available with at least mdevctl
>>> v1.3.0 installed.
>>> +
>>> * qemu: Support clusters in CPU topology
>>> It is now possible to configure the guest CPU topology to use
>>> clusters.
>>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>>> index 8413e3c19a..a8dff4a32e 100644
>>> --- a/libvirt.spec.in
>>> +++ b/libvirt.spec.in
>>> @@ -613,6 +613,7 @@ Requires: libvirt-libs = %{version}-%{release}
>>> # needed for device enumeration
>>> Requires: systemd >= 185
>>> # For managing persistent mediated devices
>>> +# Note: for nodedev-update support at least mdevctl v1.3.0 is required
>>> Requires: mdevctl
>>> # 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 baff49a0ae..5bece1a30a 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,100 @@ 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;
>>> +}
>>> +
>>> +
>>> +/* checks if mdevctl supports on the command modify the options
>>> live, defined
>>> + * and jsonfile
>>> + */
>>> +static int
>>> +nodeDeviceGetMdevctlModifySupportCheck(void)
>>> +{
>>> + int status;
>>> + g_autoptr(virCommand) cmd = NULL;
>>> + const char *subcommand =
>>> virMdevctlCommandTypeToString(MDEVCTL_CMD_MODIFY);
>>> +
>>> + cmd = virCommandNewArgList(MDEVCTL,
>>> + subcommand,
>>> + "--defined",
>>> + "--live",
>>> + "--jsonfile",
>>> + "b",
>>> + "--help",
>>> + NULL);
>>> +
>>> + if (!cmd)
>>> + return -1;
>>> +
>>> + if (virCommandRun(cmd, &status) < 0)
>>> + return -1;
>>> +
>>> + if (status != 0) // update is unsupported
>>> + return -1;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +
>>> +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 (nodeDeviceGetMdevctlModifySupportCheck() < 0) {
>>> + VIR_WARN("Installed mdevctl version does not support modify
>>> with options jsonfile, defined and live");
>>> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>>> + _("Unable to modify mediated device: modify
>>> unsupported"));
>>> + 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)
>>> @@ -2106,3 +2203,158 @@ nodeDeviceIsActive(virNodeDevice *device)
>>> virNodeDeviceObjEndAPI(&obj);
>>> return ret;
>>> }
>>> +
>>> +
>>> +static int
>>> +nodeDeviceDefCloneMdevConfigs(virNodeDeviceDef *def_upd)
>>> +{
>>> + virNodeDevCapsDef *caps = NULL;
>>> + virNodeDevCapMdev *cap_mdev_upd = NULL;
>>> + virMediatedDeviceConfig *src_mdev_config = NULL;
>>> + virMediatedDeviceConfig *dst_mdev_config = NULL;
>>> +
>>> + 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;
>>> +
>>> + src_mdev_config = &cap_mdev_upd->defined_config;
>>> + dst_mdev_config = &cap_mdev_upd->active_config;
>>> +
>>> + if (STRNEQ_NULLABLE(dst_mdev_config->type,
>>> src_mdev_config->type)) {
>>> + g_free(dst_mdev_config->type);
>>> + dst_mdev_config->type = g_strdup(src_mdev_config->type);
>>> + }
>>> +
>>> + virMediatedDeviceAttrsCopy(dst_mdev_config, src_mdev_config);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +
I'm making a couple additional suggestions here relevant to our
discussion below.
>>> +static int
>>> +nodeDeviceDefCompareMdevs(virNodeDeviceDef *def_cur,
>>> + virNodeDeviceDef *def_upd,
>>> + bool live)
I think this function is poorly named. It implies that you are comparing
two different mdevs for equality, but it's really a function that
validates whether it is valid to update from one device def to a new
def. So it would be much more understandable if it was named something
like nodeDeviceDefValidateUpdate().
In addition, I'd personally appreciate less cryptic variable names. For
example, `def_cur` could just be `def` or `current_def`. `def_upd` could
be `new_def`. Similar for the local variables below.
>>> +{
>>> + 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,
>>> + _("Cannot update device '%1$s, uuid mismatch
>>> (current uuid '%2$s')"),
>>> + def_cur->name,
>>> + cap_mdev_cur->uuid);
>>> + return -1;
>>> + }
>>> + // for a live update the types of the active configs must match!
Related to my suggestion below, you could change the comment here to
something like:
/* A live update cannot change the mdev type. Since the new config is
* stored in defined_config, compare that to the mdev type of the
* current live config to make sure they match
*/
Then it would be clear why you're comparing the current active config to
the new defined config.
>>> + if (live &&
>>> + STRNEQ_NULLABLE(cap_mdev_cur->active_config.type,
>>> cap_mdev_upd->active_config.type)) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> + _("Cannot update device '%1$s', type
>>> mismatch (current type '%2$s')"),
>>> + def_cur->name,
>>> + cap_mdev_cur->active_config.type);
>>> + return -1;
>>> + }
>>> + if (STRNEQ_NULLABLE(cap_mdev_cur->parent_addr,
>>> cap_mdev_upd->parent_addr)) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> + _("Cannot update device '%1$s', parent
>>> address mismatch (current parent address '%2$s')"),
>>> + def_cur->name,
>>> + cap_mdev_cur->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
^ another case where less cryptic variable names would be nice.
>>> + 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;
>>> +
>>> + // Before live update clone defined to active config
>>> + if (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE &&
>>> + nodeDeviceDefCloneMdevConfigs(def_upd) < 0)
>>> + goto cleanup;
>>
>> It's not clear to me why this is necessary. As far as I can tell,
>> only the defined_config is used to generate the JSON to send to
>> mdevctl. And this def is freed at the end of the function and then we
>> re-query mdevctl to update our device list.
>
> In case a live update is to be made there is a check of the mdev types
> also on the active config. Therefore I try to match the def used for
> the update to the current def by cloning the defined_config into the
> active_config. Another solution would have been to change the compare in
> nodeDeviceDefCloneMdevConfigs from
> STRNEQ_NULLABLE(cap_mdev_cur->active_config.type,
> cap_mdev_upd->active_config.type)
> into
> STRNEQ_NULLABLE(cap_mdev_cur->active_config.type,
> cap_mdev_upd->defined_config.type)
> which seems kind of wrong to me but I am willing to suffer if you
> disagree... :D
I assume you mean the function nodeDeviceDefCompareMdevs(). I do think
that would be a better approach, actually. It does look a bit weird, but
with a clear comment in the function (suggestion above), I think it's
more understandable than the alternative of calling one function to copy
the persistent config into the active config only for the purposes of
checking it in another function.
Jonathon
Yes, I meant nodeDeviceDefCompareMdevs().
I made all the changes that you suggested above.
>
>>
>>> +
>>> + // Compare def_cur and def_upd for compatibleness e.g.
>>> parent, type and uuid
>>
>> compatibleness -> compatibility
>
> Changed.
>
>>
>>> + 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"));
>>> + goto cleanup;
>>> + }
>>> +
>>> + 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/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_update.xml
b/tests/nodedevmdevctldata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_update.xml
>>> new file mode 100644
>>> index 0000000000..17e3611bf4
>>> --- /dev/null
>>> +++
>>>
b/tests/nodedevmdevctldata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_update.xml
>>> @@ -0,0 +1,16 @@
>>> +<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>
>>> + <attr name='add_attr_1' value='val1'/>
>>> + <attr name='add_attr_2' value='val2'/>
>>> + <iommuGroup number='4'/>
>>> + </capability>
>>> +</device>
>>> diff --git a/tests/nodedevmdevctldata/mdevctl-modify.argv
>>> b/tests/nodedevmdevctldata/mdevctl-modify.argv
>>> new file mode 100644
>>> index 0000000000..341c5e7fd0
>>> --- /dev/null
>>> +++ b/tests/nodedevmdevctldata/mdevctl-modify.argv
>>> @@ -0,0 +1,25 @@
>>> +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 \
>>> +--live
>>> +mdevctl \
>>> +modify \
>>> +--parent=0.0.0052 \
>>> +--jsonfile=/dev/stdin \
>>> +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \
>>> +--defined \
>>> +--live
>>> diff --git a/tests/nodedevmdevctldata/mdevctl-modify.json
>>> b/tests/nodedevmdevctldata/mdevctl-modify.json
>>> new file mode 100644
>>> index 0000000000..7ae8c73ce3
>>> --- /dev/null
>>> +++ b/tests/nodedevmdevctldata/mdevctl-modify.json
>>> @@ -0,0 +1 @@
>>>
+{"mdev_type":"vfio_ccw-io","start":"manual"}{"mdev_type":"vfio_ccw-io","start":"manual","attrs":[{"add_attr_1":"val1"},{"add_attr_2":"val2"}]}{"mdev_type":"vfio_ccw-io","start":"manual"}{"mdev_type":"vfio_ccw-io","start":"manual","attrs":[{"add_attr_1":"val1"},{"add_attr_2":"val2"}]}
>>> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
>>> index f49d668461..11273e3049 100644
>>> --- a/tests/nodedevmdevctltest.c
>>> +++ b/tests/nodedevmdevctltest.c
>>> @@ -33,7 +33,10 @@ testCommandDryRunCallback(const char *const*args
>>> G_GNUC_UNUSED,
>>> {
>>> char **stdinbuf = opaque;
>>> - *stdinbuf = g_strdup(input);
>>> + if (*stdinbuf)
>>> + *stdinbuf = g_strconcat(*stdinbuf, input, NULL);
>>
>> I think g_strjoin() would be nicer so we can add a newline separator
>> between invocations.
>
> There is no problem adding a newline seperator with the g_strconcat as
> well. Using g_strconcat kind of looks more natural to me
>
> *stdinbuf = g_strconcat(*stdinbuf, "\n", input, NULL);
>
> versus
>
> *stdinbuf = g_strjoin("\n", *stdinbuf, input, NULL);
>
>>
>>> + else
>>> + *stdinbuf = g_strdup(input);
>>> }
>>> typedef virCommand * (*MdevctlCmdFunc)(virNodeDeviceDef *, char
>>> **, char **);
>>> @@ -63,6 +66,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 +177,85 @@ testMdevctlAutostart(const void *data
>>> G_GNUC_UNUSED)
>>> return ret;
>>> }
>>> +
>>> +static int
>>> +testMdevctlModify(const void *data G_GNUC_UNUSED)
>>> +{
>>> + g_autoptr(virNodeDeviceDef) def = NULL;
>>> + g_autoptr(virNodeDeviceDef) def_update = 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;
>>> + g_autofree char *stdinbuf = NULL;
>>> + g_autofree char *mdevxml =
>>> +
>>>
g_strdup_printf("%s/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml",
>>> + abs_srcdir);
>>> + g_autofree char *mdevxml_update =
>>> +
>>>
g_strdup_printf("%s/nodedevmdevctldata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_update.xml",
>>> + abs_srcdir);
>>> + /* just concatenate both calls into the same output files */
>>> + g_autofree char *cmdlinefile =
>>> +
g_strdup_printf("%s/nodedevmdevctldata/mdevctl-modify.argv",
>>> + abs_srcdir);
>>> + g_autofree char *jsonfile =
>>> +
g_strdup_printf("%s/nodedevmdevctldata/mdevctl-modify.json",
>>> + 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,
>>> testCommandDryRunCallback, &stdinbuf);
>>> +
>>> + if (!(definedcmd = nodeDeviceGetMdevctlModifyCommand(def, true,
>>> false, &errmsg)))
>>> + goto cleanup;
>>> +
>>> + if (virCommandRun(definedcmd, NULL) < 0)
>>> + goto cleanup;
>>> +
>>> + if (!(def_update = virNodeDeviceDefParse(NULL, mdevxml_update,
>>> EXISTING_DEVICE, VIRT_TYPE,
>>> + &parser_callbacks,
>>> NULL, false)))
>>> + goto cleanup;
>>> +
>>> + if (!(livecmd = nodeDeviceGetMdevctlModifyCommand(def_update,
>>> false, true, &errmsg)))
>>> + goto cleanup;
>>> +
>>> + if (virCommandRun(livecmd, NULL) < 0)
>>> + goto cleanup;
>>> +
>>> + if (!(livecmd = nodeDeviceGetMdevctlModifyCommand(def, false,
>>> true, &errmsg)))
>>> + goto cleanup;
>>> +
>>> + if (virCommandRun(livecmd, NULL) < 0)
>>> + goto cleanup;
>>> +
>>> + if (!(bothcmd = nodeDeviceGetMdevctlModifyCommand(def_update,
>>> 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;
>>> +
>>> + if (virTestCompareToFile(stdinbuf, jsonfile) < 0)
>>> + goto cleanup;
>>> +
>>> + ret = 0;
>>> +
>>> + cleanup:
>>> + virBufferFreeAndReset(&buf);
>>> + return ret;
>>> +}
>>> +
>>> static int
>>> testMdevctlListDefined(const void *data G_GNUC_UNUSED)
>>> {
>>> @@ -457,6 +540,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 +571,8 @@ mymain(void)
>>> DO_TEST_AUTOSTART();
>>> + DO_TEST_MODIFY();
>>> +
>>> done:
>>> nodedevTestDriverFree(driver);
>>
>> Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
>
> Thanks.
>
>> _______________________________________________
>> Devel mailing list -- devel(a)lists.libvirt.org
>> To unsubscribe send an email to devel-leave(a)lists.libvirt.org
>
_______________________________________________
Devel mailing list -- devel(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)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