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

Re: [libvirt] [PATCH v2 6/8] blockcopy: add a way to parse disk source



On 08/26/14 13:21, Eric Blake wrote:
> The new blockcopy API wants to reuse only a subset of the disk
> hotplug parser - namely, we only care about the embedded
> virStorageSourcePtr inside a <disk> XML.  Strange as it may
> seem, it was easier to just parse an entire disk definition,
> then throw away everything but the embedded source, than it
> was to disentangle the source parsing code from the rest of
> the overall disk parsing function.  All that I needed was a
> couple of tweaks and a new internal flag that determines
> whether the normally-mandatory target element can be
> gracefully skipped.
> 
> While adding the new flag, I noticed we had a verify() that
> was incomplete after several recent internal flag additions;
> move that up higher in the code to make it harder to forget
> to modify on the next flag addition.
> 
> * src/conf/domain_conf.h (virDomainDiskSourceParse): New
> prototype.
> * src/conf/domain_conf.c (VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE):
> New flag.
> (virDomainDiskDefParseXML): Honor flag to make target optional.
> (virDomainDiskSourceParse): New function.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  src/conf/domain_conf.c   | 147 +++++++++++++++++++++++++++++++----------------
>  src/conf/domain_conf.h   |   4 ++
>  src/libvirt_private.syms |   1 +
>  3 files changed, 103 insertions(+), 49 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index dd512ca..2ee2af0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -89,19 +89,36 @@ struct _virDomainXMLOption {
> 
> 
>  /* Private flags used internally by virDomainSaveStatus and
> - * virDomainLoadStatus. */
> + * virDomainLoadStatus, in addition to the public virDomainXMLFlags. */
>  typedef enum {
>     /* dump internal domain status information */
> -   VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16),
> +   VIR_DOMAIN_XML_INTERNAL_STATUS = 1 << 16,
>     /* dump/parse <actual> element */
> -   VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET = (1<<17),
> +   VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET = 1 << 17,
>     /* dump/parse original states of host PCI device */
> -   VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = (1<<18),
> -   VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = (1<<19),
> -   VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = (1<<20),
> -   VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST = (1<<21),
> +   VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = 1 << 18,
> +   VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = 1 << 19,
> +   VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = 1 << 20,
> +   VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST = 1 << 21,
> +   /* parse only source half of <disk> */
> +   VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE = 1 << 22,
>  } virDomainXMLInternalFlags;
> 
> +#define DUMPXML_FLAGS                           \
> +    (VIR_DOMAIN_XML_SECURE |                    \
> +     VIR_DOMAIN_XML_INACTIVE |                  \
> +     VIR_DOMAIN_XML_UPDATE_CPU |                \
> +     VIR_DOMAIN_XML_MIGRATABLE)
> +
> +verify(((VIR_DOMAIN_XML_INTERNAL_STATUS |
> +         VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET |
> +         VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES |
> +         VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM |
> +         VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT |
> +         VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST |
> +         VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)
> +        & DUMPXML_FLAGS) == 0);
> +

Again, code tweaks and unrelated fixes ...

>  VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST,
>                "custom-argv",
>                "custom-monitor",


> @@ -10359,7 +10379,7 @@ virDomainDeviceDefParse(const char *xmlStr,
>      }
> 
>      /* callback to fill driver specific device aspects */
> -    if (virDomainDeviceDefPostParse(dev, def,  caps, xmlopt) < 0)
> +    if (virDomainDeviceDefPostParse(dev, def, caps, xmlopt) < 0)
>          goto error;

Unrelated.

> 
>   cleanup:
> @@ -10373,6 +10393,47 @@ virDomainDeviceDefParse(const char *xmlStr,
>  }
> 
> 
> +virStorageSourcePtr
> +virDomainDiskDefSourceParse(const char *xmlStr,
> +                            const virDomainDef *def,
> +                            virDomainXMLOptionPtr xmlopt,
> +                            unsigned int flags)
> +{
> +    xmlDocPtr xml;
> +    xmlNodePtr node;
> +    xmlXPathContextPtr ctxt = NULL;
> +    virDomainDiskDefPtr disk = NULL;
> +    virStorageSourcePtr ret = NULL;
> +
> +    if (!(xml = virXMLParseStringCtxt(xmlStr, _("(disk_definition)"), &ctxt)))
> +        goto cleanup;
> +    node = ctxt->node;

Is the extra variable used to convert types?

> +
> +    if (!xmlStrEqual(node->name, BAD_CAST "disk")) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("expecting root element of 'disk', not '%s'"),
> +                       node->name);
> +        goto cleanup;
> +    }
> +
> +    flags |= VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE;
> +    if (!(disk = virDomainDiskDefParseXML(xmlopt, node, ctxt,
> +                                          NULL, def->seclabels,
> +                                          def->nseclabels,
> +                                          flags)))
> +        goto cleanup;
> +
> +    ret = disk->src;
> +    disk->src = NULL;
> +
> + cleanup:
> +    virDomainDiskDefFree(disk);
> +    xmlFreeDoc(xml);
> +    xmlXPathFreeContext(ctxt);
> +    return ret;
> +}
> +
> +
>  static const char *
>  virDomainChrTargetTypeToString(int deviceType,
>                                 int targetType)
> @@ -17726,18 +17787,6 @@ virDomainHugepagesFormat(virBufferPtr buf,
>  }
> 
> 
> -#define DUMPXML_FLAGS                           \
> -    (VIR_DOMAIN_XML_SECURE |                    \
> -     VIR_DOMAIN_XML_INACTIVE |                  \
> -     VIR_DOMAIN_XML_UPDATE_CPU |                \
> -     VIR_DOMAIN_XML_MIGRATABLE)
> -
> -verify(((VIR_DOMAIN_XML_INTERNAL_STATUS |
> -         VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET |
> -         VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES |
> -         VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST)
> -        & DUMPXML_FLAGS) == 0);
> -
>  static bool
>  virDomainDefHasCapabilitiesFeatures(virDomainDefPtr def)
>  {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index aead903..512d097 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2294,6 +2294,10 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr,
>                                                virCapsPtr caps,
>                                                virDomainXMLOptionPtr xmlopt,
>                                                unsigned int flags);
> +virStorageSourcePtr virDomainDiskDefSourceParse(const char *xmlStr,
> +                                                const virDomainDef *def,
> +                                                virDomainXMLOptionPtr xmlopt,
> +                                                unsigned int flags);
>  virDomainDefPtr virDomainDefParseString(const char *xmlStr,
>                                          virCapsPtr caps,
>                                          virDomainXMLOptionPtr xmlopt,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6b9ee21..1195208 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -222,6 +222,7 @@ virDomainDiskDefAssignAddress;
>  virDomainDiskDefForeachPath;
>  virDomainDiskDefFree;
>  virDomainDiskDefNew;
> +virDomainDiskDefSourceParse;
>  virDomainDiskDeviceTypeToString;
>  virDomainDiskDiscardTypeToString;
>  virDomainDiskErrorPolicyTypeFromString;
> 

I'd really prefer the unrelated tweaks posted separately.

Again, what you have works so ACK to this patch even if you decide
against splitting it up.

Peter

Attachment: signature.asc
Description: OpenPGP digital signature


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