Re: [libvirt PATCH 4/7] nodedev: Add parser validation for node devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux