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

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

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 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) {
-        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;

  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",

      switch (virtPort->virtPortType) {
-        break;
@@ -913,9 +908,15 @@ virNetDevVPortProfileFormat(virNetDevVPortProfilePtr virtPort,
                            "<parameters profileid='%s'/>\n",
+    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.

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