[libvirt] [PATCH 12/33] Fix error reporting in port profile parsing/formatting APIs

[Laine Stump]

After applying this patch, make fails with:

   CC     libvirt_util_la-network.lo
cc1: warnings being treated as errors
util/network.c: In function 'virNetDevVPortProfileParse':
util/network.c:712:23: error: assignment makes pointer from integer 
without a cast
util/network.c:712:69: error: ordered comparison of pointer with integer 
zero [-Wextra]


On 11/03/2011 01:30 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> The virtual port profile parsing/formatting APIs do not
> correctly handle unknown profile type strings/numbers.
> They behave as a no-op, instead of raising an error

Actually I've noticed a *lot* of the *Format functions ignore the 
possibility of bad values in the vir*Def objects (e.g. invalid enum 
values), and have debated with myself about whether to ignore or report 
invalid data.


>
> * src/util/network.c, src/util/network.h: Fix error
>    handling of port profile APIs
> * src/conf/domain_conf.c, src/conf/network_conf.c: Update
>    for API changes
> ---
>   src/conf/domain_conf.c  |   16 +++++++-----
>   src/conf/network_conf.c |   20 +++++++-------
>   src/util/network.c      |   61 ++++++++++++++++++++++++-----------------------
>   src/util/network.h      |    8 +++---
>   4 files changed, 54 insertions(+), 51 deletions(-)

> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 023eb9f..5e38bee 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c

> @@ -700,13 +699,19 @@ virNetDevVPortProfileParse(xmlNodePtr node,
>
>       if (VIR_ALLOC(virtPort)<  0) {
>           virReportOOMError();
> -        return -1;
> +        return NULL;
>       }
>
>       virtPortType = virXMLPropString(node, "type");
>       if (!virtPortType) {
>           virSocketError(VIR_ERR_XML_ERROR, "%s",
> -                             _("missing virtualportprofile type"));
> +                       _("missing virtualportprofile type"));
> +        goto error;
> +    }

The following should be "virtPort->virPortType = ....":

> +
> +    if ((virtPortType = virNetDevVPortTypeFromString(virtPortType))<= 0) {
> +        virSocketError(VIR_ERR_XML_ERROR,
> +                       _("unknown virtualportprofile type %s"), virtPortType);
>           goto error;
>       }
>


> @@ -879,23 +877,20 @@ virNetDevVPortProfileEqual(virNetDevVPortProfilePtr a, virNetDevVPortProfilePtr
>       return true;
>   }
>
> -void
> +
> +int
>   virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort,
>                               virBufferPtr buf)
>   {
>       char uuidstr[VIR_UUID_STRING_BUFLEN];
>
>       if (!virtPort || virtPort->virtPortType == VIR_NETDEV_VPORT_PROFILE_NONE)
> -        return;
> +        return 0;
>
>       virBufferAsprintf(buf, "<virtualport type='%s'>\n",
>                         virNetDevVPortTypeToString(virtPort->virtPortType));
>
>       switch (virtPort->virtPortType) {
> -    case VIR_NETDEV_VPORT_PROFILE_NONE:
> -    case VIR_NETDEV_VPORT_PROFILE_LAST:
> -        break;
> -
>       case VIR_NETDEV_VPORT_PROFILE_8021QBG:
>           virUUIDFormat(virtPort->u.virtPort8021Qbg.instanceID,
>                         uuidstr);
> @@ -913,9 +908,15 @@ virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort,
>                             "<parameters profileid='%s'/>\n",
>                             virtPort->u.virtPort8021Qbh.profileID);
>           break;
> +
> +    default:
> +        virSocketError(VIR_ERR_XML_ERROR,
> +                       _("unexpected virtualport type %d"), virtPort->virtPortType);
> +        return -1;
>       }


A bit of digression:

In the example here, you're doing something with the enum value aside 
from just converting it to a string, so there's a ready place to put in 
the check and return failure if it's out of bounds/unknown, but there 
are many many places where an XXXTypeToString() macro is used directly 
as an argument in a printf with no check for validity. On one hand, this 
is a bit sloppy if we consider that the [whatever]Def objects may 
contain unverified data (downright dangerous if you think about 
platforms that don't protect against NULL dereferences in printf!); on 
the other hand, if we changed every occurrence of that to get the string 
value into a temp and check for non-NULL before using it in a printf, it 
would add significant clutter to the code (domain_conf.c seems to be the 
biggest offender here). Even in this case, we're calling 
virBufferAsprintf with the unqualified return from 
virNetDevVPortTypeToString before we eventually vet it in the following 
switch statement.

So do we consider objects sent to the Format functions to contain 
qualified data or not? If not, there's quite a large patch waiting in 
the wings :-)


Anyway, ACK with the compile problem fixed.