On 02/20/2013 12:06 PM, Peter Krempa wrote:
> This patch adds instrumentation that will ultimately allow to split out
> filling of defaults and input validation from the XML parser to separate
> functions.
>
> With this patch, after the XML is parsed, a callback to the driver is
> issued requesing to fill and validate driver specific details of the
> configuration. This allows to use sensible defaults and checks on a per
> driver basis at the time the XML is parsed.
>
> Two callback pointers are exposed in the virCaps object:
> * virDriverDeviceDefCallback - called for a single device parsed and for
> every single device in a domain config.
> A virDomainDeviceDefPtr is passed.
> * virDriverDomainDefCallback - called if a domain definition is parsed
> device specific callbacks were called.
> A virDomainDefPtr is passed.
>
> Errors may be reported in those callbacks resulting in a XML parsing
> failure.
>
> Additionally two internal filler functions are added:
> virDomainDeviceDefUpdateDefaultsInternal and
> virDomainDefUpdateDefaultsInternal that are meant to be used for
> separating the validation and defaults assignment code from the parser
> function.
> ---
> src/conf/capabilities.h | 6 +++++
> src/conf/domain_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 78 insertions(+)
>
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index cc01765..56cd09f 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -171,6 +171,12 @@ struct _virCaps {
> bool hasWideScsiBus;
> const char *defaultInitPath;
>
> +
> + /* these callbacks are called after a XML definition of a device or domain
> + * is parsed. They are meant to validate and fill driver-specific values. */
> + int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */
> + int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */
If you know that virDriverDeviceDefCallback is always given a
virDomainDeviceDefPtr, why prototype it as void*? Same question for
virDriverDomainDefCallback.
Additionally, in the callback for a device, we will need to have the
virDomainDefPtr (e.g. so that we can see what machinetype was selected
for the domain). And in both of these callbacks we will need to get the
virCapsPtr so that (for example), we can have access to information
about which devices are available for which machine types, the default
pci addresses of integrated devices, etc.
So, I think the callbacks should be like this:
int (*virDriverDeviceDefCallback) (virCapsPtr caps, virDomainDefPtr
*domdef, virDomainDeviceDefPtr *devdef);
int (*virDriverDomainDefCallback) (virCapsPtr caps, virDomainDefPtr
*domdef);
> +
> virDomainXMLNamespace ns;
> };
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 421492f..d881983 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2353,6 +2353,70 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def,
> }
>
>
> +static int
> +virDomainDeviceDefUpdateDefaultsInternal(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED)
I don't think I agree with calling these functions "UpdateDefaults",
because they could be used for any number of things, e.g. adding extra
devices, creating bridges for extra buses, validating addresses, etc.
Maybe they could be called virDomain*AdjustConfig*() or
virDomain*PostProcess*().
> +{
> + /* not in use yet */
> + return 0;
> +}
> +
> +
> +static int
> +virDomainDefUpdateDefaultsInternal(virDomainDefPtr def ATTRIBUTE_UNUSED,
> + virCapsPtr caps ATTRIBUTE_UNUSED)
> +{
> + /* not in use yet */
> + return 0;
> +}
> +
> +
> +static int
> +virDomainDeviceDefUpdateDefaults(virDomainDeviceDefPtr dev,
> + virCapsPtr caps)
> +{
> + int ret;
> + /* at first call the driver callback to update the device */
> + if (caps->virDriverDeviceDefCallback &&
> + (ret = (caps->virDriverDeviceDefCallback)(dev)) < 0)
> + return ret;
> +
> + /* then fill the parse defaults and do the checks */
> + return virDomainDeviceDefUpdateDefaultsInternal(dev);
> +}
> +
> +
> +static int
> +virDomainDefUpdateDefaultsDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED,
> + virDomainDeviceDefPtr dev,
> + virDomainDeviceInfoPtr info ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + virCapsPtr caps = opaque;
> + return virDomainDeviceDefUpdateDefaults(dev, caps);
> +}
> +
> +
> +static int
> +virDomainDefUpdateDefaults(virDomainDefPtr def,
> + virCapsPtr caps)
> +{
> + int ret;
> +
> + /* at first update all the devices , unfortunately they are separate */
> + if ((ret = virDomainDeviceInfoIterate(def,
> + virDomainDefUpdateDefaultsDeviceIterator,
> + caps)) < 0)
> + return ret;
> +
> + /* call the driver callback to update the rest of the definition */
> + if (caps->virDriverDomainDefCallback &&
> + (ret = (caps->virDriverDomainDefCallback)(def)) < 0)
> + return ret;
I find myself wondering if we're going to want to adjust the domain as a
whole first, then the devices, or the individual devices, then the
domain. I'm not sure which would be more useful. However, it does seem
like the device callback is more likely to look at info for the domain
(which we would probably want to be already adjusted at that time) than
for the domain callback to want to look at individual devices. I could
be wrong though...
> +
> + return virDomainDefUpdateDefaultsInternal(def, caps);
> +}
> +
> +
> void virDomainDefClearPCIAddresses(virDomainDefPtr def)
> {
> virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL);
> @@ -8224,6 +8288,10 @@ virDomainDeviceDefParse(virCapsPtr caps,
> goto error;
> }
>
> + /* callback to fill driver specific device aspects */
> + if (virDomainDeviceDefUpdateDefaults(dev, caps) < 0)
> + goto error;
> +
> cleanup:
> xmlFreeDoc(xml);
> xmlXPathFreeContext(ctxt);
> @@ -10714,6 +10782,10 @@ virDomainDefParseXML(virCapsPtr caps,
> if (virDomainDefAddImplicitControllers(def) < 0)
> goto error;
>
> + /* callback to fill driver specific domain aspects */
> + if (virDomainDefUpdateDefaults(def, caps) < 0)
> + goto error;
> +
> virBitmapFree(bootMap);
>
> return def;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list