On 7/23/21 6:40 PM, Jonathon Jongsma wrote:
> At the moment, this is only for mediated devices. When a new mediated
> device is created or defined, the xml is expected specify the nodedev
> name of an existing device as its parent. We were not previously
> validating this and were simply accepting any string here.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> src/conf/node_device_conf.c | 30 +++++++++---
> src/conf/node_device_conf.h | 20 +++++++-
> src/conf/virnodedeviceobj.h | 1 +
> src/hypervisor/domain_driver.c | 7 +--
> src/node_device/node_device_driver.c | 68 +++++++++++++++++++++++++++-
> src/node_device/node_device_driver.h | 3 ++
> src/node_device/node_device_udev.c | 2 +
> src/test/test_driver.c | 6 ++-
> tests/nodedevmdevctltest.c | 7 ++-
> tests/nodedevxml2xmltest.c | 3 +-
> 10 files changed, 129 insertions(+), 18 deletions(-)
>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 332b12f997..cd1c07092d 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2174,10 +2174,12 @@ static virNodeDeviceDef *
> virNodeDeviceDefParse(const char *str,
> const char *filename,
> int create,
> - const char *virt_type)
> + const char *virt_type,
> + virNodeDeviceDefParserCallbacks *parserCallbacks,
> + void *opaque)
> {
> xmlDocPtr xml;
> - virNodeDeviceDef *def = NULL;
> + g_autoptr(virNodeDeviceDef) def = NULL;
>
> if ((xml = virXMLParse(filename, str, _("(node_device_definition)")))) {
> def = virNodeDeviceDefParseNode(xml, xmlDocGetRootElement(xml),
> @@ -2185,25 +2187,39 @@ virNodeDeviceDefParse(const char *str,
> xmlFreeDoc(xml);
> }
>
> - return def;
> + if (parserCallbacks) {
> + int ret = 0;
> + /* validate definition */
> + if (parserCallbacks->validate) {
> + ret = parserCallbacks->validate(def, opaque);
> + if (ret < 0)
> + return NULL;
Or simply without the @ret variable:
if (parserCallbacks->validate &&
parserCallbacks->validate(def, opaque) < 0)
return NULL;
Maybe even a direct call of the ->validate() callback is sufficient
because looking into the future there's not a case where parserCallbacks
!= NULL but ->validate == NULL; or is there?
> + }
> + }
> +
> + return g_steal_pointer(&def);
> }
>
>
> virNodeDeviceDef *
> virNodeDeviceDefParseString(const char *str,
> int create,
> - const char *virt_type)
> + const char *virt_type,
> + virNodeDeviceDefParserCallbacks *parserCallbacks,
> + void *opaque)
> {
> - return virNodeDeviceDefParse(str, NULL, create, virt_type);
> + return virNodeDeviceDefParse(str, NULL, create, virt_type, parserCallbacks, opaque);
> }
>
>
> virNodeDeviceDef *
> virNodeDeviceDefParseFile(const char *filename,
> int create,
> - const char *virt_type)
> + const char *virt_type,
> + virNodeDeviceDefParserCallbacks *parserCallbacks,
> + void *opaque)
> {
> - return virNodeDeviceDefParse(NULL, filename, create, virt_type);
> + return virNodeDeviceDefParse(NULL, filename, create, virt_type, parserCallbacks, opaque);
> }
>
>
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index 556878b9a8..786de85060 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -349,15 +349,31 @@ struct _virNodeDeviceDef {
> char *
> virNodeDeviceDefFormat(const virNodeDeviceDef *def);
>
> +
> +typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef *dev,
> + void *opaque);
> +
> +typedef int (*virNodeDeviceDefValidateCallback)(virNodeDeviceDef *dev,
> + void *opaque);
> +
> +typedef struct _virNodeDeviceDefParserCallbacks {
> + virNodeDeviceDefPostParseCallback postParse;
> + virNodeDeviceDefValidateCallback validate;
> +} virNodeDeviceDefParserCallbacks;
> +
> virNodeDeviceDef *
> virNodeDeviceDefParseString(const char *str,
> int create,
> - const char *virt_type);
> + const char *virt_type,
> + virNodeDeviceDefParserCallbacks *callbacks,
> + void *opaque);
>
> virNodeDeviceDef *
> virNodeDeviceDefParseFile(const char *filename,
> int create,
> - const char *virt_type);
> + const char *virt_type,
> + virNodeDeviceDefParserCallbacks *callbacks,
> + void *opaque);
>
> virNodeDeviceDef *
> virNodeDeviceDefParseNode(xmlDocPtr xml,
> diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h
> index 0cb78748a4..1fdd4f65da 100644
> --- a/src/conf/virnodedeviceobj.h
> +++ b/src/conf/virnodedeviceobj.h
> @@ -47,6 +47,7 @@ struct _virNodeDeviceDriverState {
>
> /* Immutable pointer, self-locking APIs */
> virObjectEventState *nodeDeviceEventState;
> + virNodeDeviceDefParserCallbacks parserCallbacks;
> };
>
> void
> diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> index 29e11c0447..2969d55173 100644
> --- a/src/hypervisor/domain_driver.c
> +++ b/src/hypervisor/domain_driver.c
> @@ -395,7 +395,8 @@ virDomainDriverNodeDeviceReset(virNodeDevicePtr dev,
> if (!xml)
> return -1;
>
> - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
> + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL,
> + NULL, NULL);
> if (!def)
> return -1;
>
> @@ -440,7 +441,7 @@ virDomainDriverNodeDeviceReAttach(virNodeDevicePtr dev,
> if (!xml)
> return -1;
>
> - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
> + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL, NULL, NULL);
> if (!def)
> return -1;
>
> @@ -488,7 +489,7 @@ virDomainDriverNodeDeviceDetachFlags(virNodeDevicePtr dev,
> if (!xml)
> return -1;
>
> - def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL);
> + def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL, NULL, NULL);
> if (!def)
> return -1;
>
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index ad2ca2a614..8f39d79c68 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -896,7 +896,8 @@ nodeDeviceCreateXML(virConnectPtr conn,
>
> virt_type = virConnectGetType(conn);
>
> - if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type)))
> + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type,
> + &driver->parserCallbacks, NULL)))
> return NULL;
>
> if (virNodeDeviceCreateXMLEnsureACL(conn, def) < 0)
> @@ -1376,7 +1377,8 @@ nodeDeviceDefineXML(virConnect *conn,
>
> virt_type = virConnectGetType(conn);
>
> - if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type)))
> + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type,
> + &driver->parserCallbacks, NULL)))
> return NULL;
>
> if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0)
> @@ -1761,3 +1763,65 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>
> return ret;
> }
> +
> +
> +/* validate that parent exists */
> +static int nodeDeviceDefValidateMdev(virNodeDeviceDef *def,
> + G_GNUC_UNUSED virNodeDevCapMdev *mdev,
> + G_GNUC_UNUSED void *opaque)
> +{
> + virNodeDeviceObj *obj = NULL;
> + if (!def->parent) {
Here and elsewhere in the series - please separate these two blocks with
an empty line:
virNodeDeviceObj *obj = NULL;
if (!def->parent)
...
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("missing parent device"));
> + return -1;
> + }
> + obj = virNodeDeviceObjListFindByName(driver->devs, def->parent);
> + if (!obj) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid parent device '%s'"),
> + def->parent);
> + return -1;
> + }
> +
> + virNodeDeviceObjEndAPI(&obj);
> + return 0;
> +}
> +
Michal
