virNWFilterRuleDefDetailsFormat: Refactor formatter

Format the rule attributes in two passes, first for positive 'match' and
second pass for negative. This removes the crazy logic for switching
between match modes inside the formatter.

The refactor makes it also more clear in which cases we actually do
format something.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index e6f7c0f8b76b7b235c2ba8e127f880bf49022195..f96ae707f97fe70d0f0071e749503fb134fd3b4e 100644 (file)
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2701,146 +2701,119 @@ static void
 virNWFilterRuleDefDetailsFormat(virBuffer *buf,
                                 const char *type,
                                 const virXMLAttr2Struct *att,
-                                virNWFilterRuleDef *def)
+                                virNWFilterRuleDef *def,
+                                bool negative,
+                                bool *hasAttrs)
 {
-    size_t i = 0, j;
-    bool typeShown = false;
-    bool neverShown = true;
-    bool asHex;
-    enum match {
-        MATCH_NONE = 0,
-        MATCH_YES,
-        MATCH_NO
-    } matchShown = MATCH_NONE;
-    nwItemDesc *item;
+    g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+    bool present = false;
+    size_t i;
+    size_t j;
 
-    while (att[i].name) {
-        virNWFilterEntryItemFlags flags;
+    if (negative)
+        virBufferAddLit(&attrBuf, " match='no'");
+
+    for (i = 0; att[i].name; i++) {
+        nwItemDesc *item;
 
         VIR_WARNINGS_NO_CAST_ALIGN
         item = (nwItemDesc *)((char *)def + att[i].dataIdx);
         VIR_WARNINGS_RESET
 
-        flags = item->flags;
-        if ((flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS)) {
-            if (!typeShown) {
-                virBufferAsprintf(buf, "<%s", type);
-                typeShown = true;
-                neverShown = false;
-            }
+        if (!(item->flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS))
+            continue;
 
-            if ((flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG)) {
-                if (matchShown == MATCH_NONE) {
-                    virBufferAddLit(buf, " match='no'");
-                    matchShown = MATCH_NO;
-                } else if (matchShown == MATCH_YES) {
-                    virBufferAddLit(buf, "/>\n");
-                    typeShown = false;
-                    matchShown = MATCH_NONE;
-                    continue;
-                }
-            } else {
-                if (matchShown == MATCH_NO) {
-                    virBufferAddLit(buf, "/>\n");
-                    typeShown = false;
-                    matchShown = MATCH_NONE;
-                    continue;
-                }
-                matchShown = MATCH_YES;
+        if (negative != !!(item->flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG))
+            continue;
+
+        present = true;
+        *hasAttrs = true;
+
+        virBufferAsprintf(&attrBuf, " %s='", att[i].name);
+
+        if (att[i].formatter && !(item->flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) {
+            if (!att[i].formatter(&attrBuf, def, item)) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("formatter for %1$s %2$s reported error"),
+                               type, att[i].name);
+                return;
             }
+        } else if ((item->flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) {
+            virBufferAddChar(&attrBuf, '$');
+            virNWFilterVarAccessPrint(item->varAccess, &attrBuf);
+        } else {
 
-            virBufferAsprintf(buf, " %s='",
-                              att[i].name);
-            if (att[i].formatter && !(flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) {
-               if (!att[i].formatter(buf, def, item)) {
-                  virReportError(VIR_ERR_INTERNAL_ERROR,
-                                 _("formatter for %1$s %2$s reported error"),
-                                 type,
-                                 att[i].name);
-                   return;
-               }
-            } else if ((flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) {
-                virBufferAddChar(buf, '$');
-                virNWFilterVarAccessPrint(item->varAccess, buf);
-            } else {
-               asHex = false;
-
-               switch (item->datatype) {
-
-               case DATATYPE_UINT8_HEX:
-                   asHex = true;
-                   G_GNUC_FALLTHROUGH;
-               case DATATYPE_IPMASK:
-               case DATATYPE_IPV6MASK:
-                   /* display all masks in CIDR format */
-               case DATATYPE_UINT8:
-                   virBufferAsprintf(buf, asHex ? "0x%x" : "%d",
-                                     item->u.u8);
-               break;
-
-               case DATATYPE_UINT16_HEX:
-                   asHex = true;
-                   G_GNUC_FALLTHROUGH;
-               case DATATYPE_UINT16:
-                   virBufferAsprintf(buf, asHex ? "0x%x" : "%d",
-                                     item->u.u16);
-               break;
-
-               case DATATYPE_UINT32_HEX:
-                   asHex = true;
-                   G_GNUC_FALLTHROUGH;
-               case DATATYPE_UINT32:
-                   virBufferAsprintf(buf, asHex ? "0x%x" : "%u",
-                                     item->u.u32);
-               break;
-
-               case DATATYPE_IPADDR:
-               case DATATYPE_IPV6ADDR:
-                   virNWIPAddressFormat(buf,
-                                        &item->u.ipaddr);
-               break;
-
-               case DATATYPE_MACMASK:
-               case DATATYPE_MACADDR:
-                   for (j = 0; j < 6; j++)
-                       virBufferAsprintf(buf, "%02x%s",
-                                         item->u.macaddr.addr[j],
-                                         (j < 5) ? ":" : "");
-               break;
-
-               case DATATYPE_STRINGCOPY:
-                   virBufferEscapeString(buf, "%s", item->u.string);
-               break;
-
-               case DATATYPE_BOOLEAN:
-                   if (item->u.boolean)
-                       virBufferAddLit(buf, "true");
-                   else
-                       virBufferAddLit(buf, "false");
-               break;
-
-               case DATATYPE_IPSETNAME:
-               case DATATYPE_IPSETFLAGS:
-               case DATATYPE_STRING:
-               case DATATYPE_LAST:
-               default:
-                   virBufferAsprintf(buf,
-                                     "UNSUPPORTED DATATYPE 0x%02x\n",
-                                     att[i].datatype);
-               }
+            switch (item->datatype) {
+
+            case DATATYPE_UINT8_HEX:
+                virBufferAsprintf(&attrBuf, "0x%x", item->u.u8);
+                break;
+
+            case DATATYPE_IPMASK:
+            case DATATYPE_IPV6MASK:
+                /* display all masks in CIDR format */
+            case DATATYPE_UINT8:
+                virBufferAsprintf(&attrBuf, "%d", item->u.u8);
+                break;
+
+            case DATATYPE_UINT16_HEX:
+                virBufferAsprintf(&attrBuf, "0x%x", item->u.u16);
+                break;
+
+            case DATATYPE_UINT16:
+                virBufferAsprintf(&attrBuf, "%d", item->u.u16);
+                break;
+
+            case DATATYPE_UINT32_HEX:
+                virBufferAsprintf(&attrBuf, "0x%x", item->u.u32);
+                break;
+
+            case DATATYPE_UINT32:
+                virBufferAsprintf(&attrBuf, "%u", item->u.u32);
+                break;
+
+            case DATATYPE_IPADDR:
+            case DATATYPE_IPV6ADDR:
+                virNWIPAddressFormat(&attrBuf, &item->u.ipaddr);
+                break;
+
+            case DATATYPE_MACMASK:
+            case DATATYPE_MACADDR:
+                for (j = 0; j < 6; j++)
+                    virBufferAsprintf(&attrBuf, "%02x%s",
+                                      item->u.macaddr.addr[j],
+                                      (j < 5) ? ":" : "");
+                break;
+
+            case DATATYPE_STRINGCOPY:
+                virBufferEscapeString(&attrBuf, "%s", item->u.string);
+                break;
+
+            case DATATYPE_BOOLEAN:
+                if (item->u.boolean)
+                    virBufferAddLit(&attrBuf, "true");
+                else
+                    virBufferAddLit(&attrBuf, "false");
+                break;
+
+            case DATATYPE_IPSETNAME:
+            case DATATYPE_IPSETFLAGS:
+            case DATATYPE_STRING:
+            case DATATYPE_LAST:
+            default:
+                virBufferAsprintf(&attrBuf,
+                                  "UNSUPPORTED DATATYPE 0x%02x\n",
+                                  att[i].datatype);
             }
-            virBufferAddLit(buf, "'");
         }
-        i++;
+
+        virBufferAddLit(&attrBuf, "'");
     }
-    if (typeShown)
-       virBufferAddLit(buf, "/>\n");
 
-    if (neverShown)
-       virBufferAsprintf(buf,
-                         "<%s/>\n", type);
+    if (!present)
+        return;
 
-    return;
+    virXMLFormatElement(buf, type, &attrBuf, NULL);
 }
 
 
@@ -2861,10 +2834,17 @@ virNWFilterRuleDefFormat(virBuffer *buf,
         virBufferAddLit(&attrBuf, " statematch='false'");
 
     for (i = 0; virAttr[i].id; i++) {
+        bool hasAttrs = false;
+
         if (virAttr[i].prtclType != def->prtclType)
             continue;
 
-        virNWFilterRuleDefDetailsFormat(&childBuf, virAttr[i].id, virAttr[i].att, def);
+        virNWFilterRuleDefDetailsFormat(&childBuf, virAttr[i].id, virAttr[i].att, def, false, &hasAttrs);
+        virNWFilterRuleDefDetailsFormat(&childBuf, virAttr[i].id, virAttr[i].att, def, true, &hasAttrs);
+
+        if (!hasAttrs)
+            virBufferAsprintf(&childBuf, "<%s/>\n", virAttr[i].id);
+
         break;
     }
 

diff --git a/tests/nwfilterxml2xmlout/quirks-invalid.xml b/tests/nwfilterxml2xmlout/quirks-invalid.xml
index f244d45e080a93c67a55ae1672957e5c30888229..5159eaf21dce87e8b26a993e10af857b9b8725ae 100644 (file)
--- a/tests/nwfilterxml2xmlout/quirks-invalid.xml
+++ b/tests/nwfilterxml2xmlout/quirks-invalid.xml
@@ -1,7 +1,7 @@
 <filter name='testcase' chain='root'>
   <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid>
   <rule action='accept' direction='in' priority='100'>
-    <tcp match='no' srcipaddr='10.1.2.3' srcipmask='32' srcportstart='22' dstportstart='22'/>
     <tcp comment='comment'/>
+    <tcp match='no' srcipaddr='10.1.2.3' srcipmask='32' srcportstart='22' dstportstart='22'/>
   </rule>
 </filter>