Re: [PATCHv2 2/9] util: eliminate union in virNetDevVPortProfile

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

 



This looks good to me.

Acked-by: Kyle Mestery <kmestery@xxxxxxxxx>

On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:

> virNetDevVPortProfile has (had) a type field that can be set to one of
> several values, and a union of several structs, one for each
> type. When a domain's interface object is of type "network", the
> domain config may not know beforehand which type of virtualport is
> going to be provided in the actual device handed down from the network
> driver at runtime, but may want to set some values in the virtualport
> that may or may not be used, depending on the type. To support this
> usage, this patch replaces the union of structs with toplevel fields
> in the struct, making it possible for all of the fields to be set at
> the same time.
> ---
> src/conf/netdev_vport_profile_conf.c | 38 ++++++++++++++++++------------------
> src/util/virnetdevopenvswitch.c      |  8 ++++----
> src/util/virnetdevvportprofile.c     | 24 +++++++++++------------
> src/util/virnetdevvportprofile.h     | 27 ++++++++++++-------------
> 4 files changed, 47 insertions(+), 50 deletions(-)
> 
> diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c
> index d008042..31ee9b4 100644
> --- a/src/conf/netdev_vport_profile_conf.c
> +++ b/src/conf/netdev_vport_profile_conf.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2009-2011 Red Hat, Inc.
> + * Copyright (C) 2009-2012 Red Hat, Inc.
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
> @@ -100,7 +100,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
>                 goto error;
>             }
> 
> -            virtPort->u.virtPort8021Qbg.managerID = (uint8_t)val;
> +            virtPort->managerID = (uint8_t)val;
> 
>             if (virStrToLong_ui(virtPortTypeID, NULL, 0, &val)) {
>                 virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -114,7 +114,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
>                 goto error;
>             }
> 
> -            virtPort->u.virtPort8021Qbg.typeID = (uint32_t)val;
> +            virtPort->typeID = (uint32_t)val;
> 
>             if (virStrToLong_ui(virtPortTypeIDVersion, NULL, 0, &val)) {
>                 virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -128,17 +128,17 @@ virNetDevVPortProfileParse(xmlNodePtr node)
>                 goto error;
>             }
> 
> -            virtPort->u.virtPort8021Qbg.typeIDVersion = (uint8_t)val;
> +            virtPort->typeIDVersion = (uint8_t)val;
> 
>             if (virtPortInstanceID != NULL) {
>                 if (virUUIDParse(virtPortInstanceID,
> -                                 virtPort->u.virtPort8021Qbg.instanceID)) {
> +                                 virtPort->instanceID)) {
>                     virReportError(VIR_ERR_XML_ERROR, "%s",
>                                          _("cannot parse instanceid parameter as a uuid"));
>                     goto error;
>                 }
>             } else {
> -                if (virUUIDGenerate(virtPort->u.virtPort8021Qbg.instanceID)) {
> +                if (virUUIDGenerate(virtPort->instanceID)) {
>                     virReportError(VIR_ERR_XML_ERROR, "%s",
>                                          _("cannot generate a random uuid for instanceid"));
>                     goto error;
> @@ -156,7 +156,7 @@ virNetDevVPortProfileParse(xmlNodePtr node)
> 
>     case VIR_NETDEV_VPORT_PROFILE_8021QBH:
>         if (virtPortProfileID != NULL) {
> -            if (virStrcpyStatic(virtPort->u.virtPort8021Qbh.profileID,
> +            if (virStrcpyStatic(virtPort->profileID,
>                                 virtPortProfileID) != NULL) {
>                 virtPort->virtPortType = VIR_NETDEV_VPORT_PROFILE_8021QBH;
>             } else {
> @@ -173,13 +173,13 @@ virNetDevVPortProfileParse(xmlNodePtr node)
>     case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
>         if (virtPortInterfaceID != NULL) {
>             if (virUUIDParse(virtPortInterfaceID,
> -                             virtPort->u.openvswitch.interfaceID)) {
> +                             virtPort->interfaceID)) {
>                 virReportError(VIR_ERR_XML_ERROR, "%s",
>                                _("cannot parse interfaceid parameter as a uuid"));
>                 goto error;
>             }
>         } else {
> -            if (virUUIDGenerate(virtPort->u.openvswitch.interfaceID)) {
> +            if (virUUIDGenerate(virtPort->interfaceID)) {
>                 virReportError(VIR_ERR_XML_ERROR, "%s",
>                                _("cannot generate a random uuid for interfaceid"));
>                 goto error;
> @@ -187,14 +187,14 @@ virNetDevVPortProfileParse(xmlNodePtr node)
>         }
>         /* profileid is not mandatory for Open vSwitch */
>         if (virtPortProfileID != NULL) {
> -            if (virStrcpyStatic(virtPort->u.openvswitch.profileID,
> +            if (virStrcpyStatic(virtPort->profileID,
>                                 virtPortProfileID) == NULL) {
>                 virReportError(VIR_ERR_XML_ERROR, "%s",
>                                _("profileid parameter too long"));
>                 goto error;
>             }
>         } else {
> -            virtPort->u.openvswitch.profileID[0] = '\0';
> +            virtPort->profileID[0] = '\0';
>         }
>         break;
> 
> @@ -234,33 +234,33 @@ virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort,
> 
>     switch (virtPort->virtPortType) {
>     case VIR_NETDEV_VPORT_PROFILE_8021QBG:
> -        virUUIDFormat(virtPort->u.virtPort8021Qbg.instanceID,
> +        virUUIDFormat(virtPort->instanceID,
>                       uuidstr);
>         virBufferAsprintf(buf,
>                           "  <parameters managerid='%d' typeid='%d' "
>                           "typeidversion='%d' instanceid='%s'/>\n",
> -                          virtPort->u.virtPort8021Qbg.managerID,
> -                          virtPort->u.virtPort8021Qbg.typeID,
> -                          virtPort->u.virtPort8021Qbg.typeIDVersion,
> +                          virtPort->managerID,
> +                          virtPort->typeID,
> +                          virtPort->typeIDVersion,
>                           uuidstr);
>         break;
> 
>     case VIR_NETDEV_VPORT_PROFILE_8021QBH:
>         virBufferAsprintf(buf,
>                           "  <parameters profileid='%s'/>\n",
> -                          virtPort->u.virtPort8021Qbh.profileID);
> +                          virtPort->profileID);
>         break;
> 
>     case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
> -        virUUIDFormat(virtPort->u.openvswitch.interfaceID,
> +        virUUIDFormat(virtPort->interfaceID,
>                       uuidstr);
> -        if (virtPort->u.openvswitch.profileID[0] == '\0') {
> +        if (virtPort->profileID[0] == '\0') {
>             virBufferAsprintf(buf, "  <parameters interfaceid='%s'/>\n",
>                               uuidstr);
>         } else {
>             virBufferAsprintf(buf, "  <parameters interfaceid='%s' "
>                               "profileid='%s'/>\n", uuidstr,
> -                              virtPort->u.openvswitch.profileID);
> +                              virtPort->profileID);
>         }
> 
>         break;
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index 7e0beef..b57532b 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>     char *vmid_ex_id = NULL;
> 
>     virMacAddrFormat(macaddr, macaddrstr);
> -    virUUIDFormat(ovsport->u.openvswitch.interfaceID, ifuuidstr);
> +    virUUIDFormat(ovsport->interfaceID, ifuuidstr);
>     virUUIDFormat(vmuuid, vmuuidstr);
> 
>     if (virAsprintf(&attachedmac_ex_id, "external-ids:attached-mac=\"%s\"",
> @@ -71,14 +71,14 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>     if (virAsprintf(&vmid_ex_id, "external-ids:vm-id=\"%s\"",
>                     vmuuidstr) < 0)
>         goto out_of_memory;
> -    if (ovsport->u.openvswitch.profileID[0] != '\0') {
> +    if (ovsport->profileID[0] != '\0') {
>         if (virAsprintf(&profile_ex_id, "external-ids:port-profile=\"%s\"",
> -                        ovsport->u.openvswitch.profileID) < 0)
> +                        ovsport->profileID) < 0)
>             goto out_of_memory;
>     }
> 
>     cmd = virCommandNew(OVSVSCTL);
> -    if (ovsport->u.openvswitch.profileID[0] == '\0') {
> +    if (ovsport->profileID[0] == '\0') {
>         virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
>                         brname, ifname,
>                         "--", "set", "Interface", ifname, attachedmac_ex_id,
> diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
> index 506240b..af9151e 100644
> --- a/src/util/virnetdevvportprofile.c
> +++ b/src/util/virnetdevvportprofile.c
> @@ -95,15 +95,15 @@ virNetDevVPortProfileEqual(virNetDevVPortProfilePtr a, virNetDevVPortProfilePtr
>         break;
> 
>     case VIR_NETDEV_VPORT_PROFILE_8021QBG:
> -        if (a->u.virtPort8021Qbg.managerID != b->u.virtPort8021Qbg.managerID ||
> -            a->u.virtPort8021Qbg.typeID != b->u.virtPort8021Qbg.typeID ||
> -            a->u.virtPort8021Qbg.typeIDVersion != b->u.virtPort8021Qbg.typeIDVersion ||
> -            memcmp(a->u.virtPort8021Qbg.instanceID, b->u.virtPort8021Qbg.instanceID, VIR_UUID_BUFLEN) != 0)
> +        if (a->managerID != b->managerID ||
> +            a->typeID != b->typeID ||
> +            a->typeIDVersion != b->typeIDVersion ||
> +            memcmp(a->instanceID, b->instanceID, VIR_UUID_BUFLEN) != 0)
>             return false;
>         break;
> 
>     case VIR_NETDEV_VPORT_PROFILE_8021QBH:
> -        if (STRNEQ(a->u.virtPort8021Qbh.profileID, b->u.virtPort8021Qbh.profileID))
> +        if (STRNEQ(a->profileID, b->profileID))
>             return false;
>         break;
> 
> @@ -637,8 +637,8 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
>     int rc = -1;
>     int op = PORT_REQUEST_ASSOCIATE;
>     struct ifla_port_vsi portVsi = {
> -        .vsi_mgr_id       = virtPort->u.virtPort8021Qbg.managerID,
> -        .vsi_type_version = virtPort->u.virtPort8021Qbg.typeIDVersion,
> +        .vsi_mgr_id       = virtPort->managerID,
> +        .vsi_type_version = virtPort->typeIDVersion,
>     };
>     bool nltarget_kernel = false;
>     int vlanid;
> @@ -658,9 +658,9 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
>     if (vlanid < 0)
>         vlanid = 0;
> 
> -    portVsi.vsi_type_id[2] = virtPort->u.virtPort8021Qbg.typeID >> 16;
> -    portVsi.vsi_type_id[1] = virtPort->u.virtPort8021Qbg.typeID >> 8;
> -    portVsi.vsi_type_id[0] = virtPort->u.virtPort8021Qbg.typeID;
> +    portVsi.vsi_type_id[2] = virtPort->typeID >> 16;
> +    portVsi.vsi_type_id[1] = virtPort->typeID >> 8;
> +    portVsi.vsi_type_id[0] = virtPort->typeID;
> 
>     switch (virtPortOp) {
>     case VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE:
> @@ -684,7 +684,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
>                                        vlanid,
>                                        NULL,
>                                        &portVsi,
> -                                       virtPort->u.virtPort8021Qbg.instanceID,
> +                                       virtPort->instanceID,
>                                        NULL,
>                                        vf,
>                                        op,
> @@ -749,7 +749,7 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname,
>                                            nltarget_kernel,
>                                            macaddr,
>                                            vlanid,
> -                                           virtPort->u.virtPort8021Qbh.profileID,
> +                                           virtPort->profileID,
>                                            NULL,
>                                            vm_uuid,
>                                            hostuuid,
> diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h
> index f33da18..3c16bfe 100644
> --- a/src/util/virnetdevvportprofile.h
> +++ b/src/util/virnetdevvportprofile.h
> @@ -59,21 +59,18 @@ typedef struct _virNetDevVPortProfile virNetDevVPortProfile;
> typedef virNetDevVPortProfile *virNetDevVPortProfilePtr;
> struct _virNetDevVPortProfile {
>     enum virNetDevVPortProfile   virtPortType;
> -    union {
> -        struct {
> -            uint8_t       managerID;
> -            uint32_t      typeID; /* 24 bit valid */
> -            uint8_t       typeIDVersion;
> -            unsigned char instanceID[VIR_UUID_BUFLEN];
> -        } virtPort8021Qbg;
> -        struct {
> -            char          profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
> -        } virtPort8021Qbh;
> -        struct {
> -            unsigned char interfaceID[VIR_UUID_BUFLEN];
> -            char          profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
> -        } openvswitch;
> -    } u;
> +    /* these members are used when virtPortType == 802.1Qbg */
> +    uint8_t       managerID;
> +    uint32_t      typeID; /* 24 bit valid */
> +    uint8_t       typeIDVersion;
> +    unsigned char instanceID[VIR_UUID_BUFLEN];
> +
> +    /* this member is used when virtPortType == 802.1Qbh|openvswitch */
> +    char          profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
> +
> +    /* this member is used when virtPortType == openvswitch */
> +    unsigned char interfaceID[VIR_UUID_BUFLEN];
> +    /* NB - if virtPortType == NONE, any/all of the items could be used */
> };
> 
> 
> -- 
> 1.7.11.2
> 
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[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]