[libvirt] [PATCH 07/10] network: new XML to support virtual switch functionality

[Eric Blake]

On 07/05/2011 01:45 AM, Laine Stump wrote:
> This implements the changes to <network> and domain <interface> XML that
> were earlier specified in the RNG.
> 
> +++ b/include/libvirt/libvirt.h.in
> @@ -1112,6 +1112,8 @@ typedef enum {
>      VIR_DOMAIN_XML_SECURE       = (1 << 0), /* dump security sensitive information too */
>      VIR_DOMAIN_XML_INACTIVE     = (1 << 1), /* dump inactive domain information */
>      VIR_DOMAIN_XML_UPDATE_CPU   = (1 << 2), /* update guest CPU requirements according to host CPU */
> +    VIR_DOMAIN_XML_RESERVED1    = (1 << 30), /* reserved for internal used */
> +    VIR_DOMAIN_XML_RESERVED2    = (1 << 31), /* reserved for internal used */

If we keep this, then s/used/use/

However, I agree with Dan that we don't want to list this in the public
header.  And even if you can convince me that we need to consume bits
from the public flags, I don't see any code in either daemon/remote.c or
src/libvirt.c that explicitly rejects any attempts to invoke an API with
one of these reserved bits set (that is, if you do merge the internal
flags onto a public field, then you had better make sure that no one
externally can abuse those flags; whereas if you use a second
internalFlags or bool arguments, you have isolated internal flags to be
completely independent of the public API).

> +
> +    type = virXMLPropString(node, "type");
> +    if (!type) {
> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                             _("missing type attribute in interface's <actual> element"));
> +        goto error;
> +    }
> +    if ((int)((*actual)->type = virDomainNetTypeFromString(type)) < 0) {

Why is this cast needed?  Oh, because struct _virDomainNetDef used 'enum
virDomainNetType type;' instead of the more typical 'int type; /* enum
virDomainNetType */'

> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                             _("unknown type '%s' in interface's <actual> element"), type);
> +        goto error;
> +    }
> +    if ((*actual)->type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
> +        (*actual)->type != VIR_DOMAIN_NET_TYPE_DIRECT) {

This is a lot of dereferencing through *actual.  It might be easier to
delay the dereferencing to the very end, by writing:

static int
virDomainActualNetDefParseXML(xmlNodePtr node,
                              xmlXPathContextPtr ctxt,
                              virDomainActualNetDefPtr *actual)
{
    virDomainActualNetDefPtr def = NULL;

    if (VIR_ALLOC(def) < 0) {
        virReportOOMError();
        goto cleanup;
    }

...
    if (def->type != VIR_DOMAIN_NET_TYPE_BRIDGE && ....

cleanup:
    ...
    if (ret < 0) {
        virDomainActualNetDefPtrFree(def);
        def = NULL;
    }
    *actual = def;
    return ret;
}

> +
> +    if ((*actual)->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +
> +        (*actual)->data.bridge.brname = virXPathString("string(./source[1]/@bridge)",
> +                                                       ctxt);
> +
> +    } else if ((*actual)->type == VIR_DOMAIN_NET_TYPE_DIRECT) {

Nit: those two blank lines don't quite match style elsewhere in the file.

> @@ -9871,7 +10063,9 @@ int virDomainSaveStatus(virCapsPtr caps,
>                          const char *statusDir,
>                          virDomainObjPtr obj)
>  {
> -    int flags = VIR_DOMAIN_XML_SECURE|VIR_DOMAIN_XML_INTERNAL_STATUS;
> +    int flags = VIR_DOMAIN_XML_SECURE |
> +       VIR_DOMAIN_XML_INTERNAL_STATUS |
> +       VIR_DOMAIN_XML_ACTUAL_NET;

This indentation looks unusual (7 spaces, but no prior hanging '(' to be
flush with).  I tend to write expressions like this as:

    int flags = (VIR_DOMAIN_XML_SECURE |
                 VIR_DOMAIN_XML_INTERNAL_STATUS |
                 VIR_DOMAIN_XML_ACTUAL_NET);

Also, there may be some merge conflicts if my patch series for cleaning
up flags usage goes in first.

> +++ b/src/conf/domain_conf.h
> @@ -1,3 +1,4 @@
> +
>  /*

Why the spurious addition of a blank leading line?

> +        case VIR_NETWORK_FORWARD_PASSTHROUGH:
> +            if (def->bridge) {
> +                virNetworkReportError(VIR_ERR_XML_ERROR,
> +                                      _("bridge name not allowed in %s mode (network '%s'"),
> +                                      virNetworkForwardTypeToString(def->forwardType),
> +                                      def->name);
> +                goto error;
> +            }
> +            /* Fall through to next case */

I'm not sure whether Coverity will recognize that spelling, so here's
hoping.  A quick 'git grep -iC2 "fall.\\?thr"' found existing spellings
that Coverity appears to honor:

/* fallthrough */
/* fall through */
/* Fallthrough */

but I'd have to actually check Coverity source to see what the canonical
list is.

> +        /* Duplicate the first interface from the pool into <forward
> +         * dev=xxx for convenience.

Missing the paired > in the comment.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110708/158b8649/attachment-0001.sig>