[PATCH V1 9/9] Improve error reporting of failures to apply filtering rules

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

 



Display the executed command and failure message if a command failed to
execute.

Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>

---
 src/nwfilter/nwfilter_ebiptables_driver.c |   82 ++++++++++++++++++------------
 1 file changed, 50 insertions(+), 32 deletions(-)

Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -63,10 +63,10 @@
 #define CMD_DEF_PRE  "cmd='"
 #define CMD_DEF_POST "'"
 #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST
-#define CMD_EXEC   "eval res=\\$\\(\"${cmd}\"\\)" CMD_SEPARATOR
+#define CMD_EXEC   "eval res=\\$\\(\"${cmd} 2>&1 \"\\)" CMD_SEPARATOR
 #define CMD_STOPONERR(X) \
     X ? "if [ $? -ne 0 ]; then" \
-        "  echo \"Failure to execute command '${cmd}'.\";" \
+        "  echo \"Failure to execute command '${cmd}' : '${res}'.\";" \
         "  exit 1;" \
         "fi" CMD_SEPARATOR \
       : ""
@@ -2785,12 +2785,16 @@ err_exit:
  */
 static int
 ebiptablesExecCLI(virBufferPtr buf,
-                  int *status)
+                  int *status, char **errbuf)
 {
     char *cmds;
     char *filename;
     int rc;
     const char *argv[] = {NULL, NULL};
+    virCommandPtr cmd;
+
+    if (errbuf)
+        VIR_FREE(*errbuf);
 
     if (virBufferError(buf)) {
         virReportOOMError();
@@ -2817,7 +2821,13 @@ ebiptablesExecCLI(virBufferPtr buf,
 
     virMutexLock(&execCLIMutex);
 
-    rc = virRun(argv, status);
+    cmd = virCommandNewArgs(argv);
+    if (errbuf)
+        virCommandSetOutputBuffer(cmd, errbuf);
+
+    rc = virCommandRun(cmd, status);
+
+    virCommandFree(cmd);
 
     virMutexUnlock(&execCLIMutex);
 
@@ -2831,6 +2841,8 @@ ebiptablesExecCLI(virBufferPtr buf,
     }
 
     VIR_DEBUG("rc = %d, status = %d", rc, *status);
+    if (errbuf && *errbuf)
+        VIR_DEBUG("error message: %s", *errbuf);
 
     unlink(filename);
 
@@ -3285,7 +3297,7 @@ ebtablesApplyBasicRules(const char *ifna
     ebtablesLinkTmpRootChain(&buf, 1, ifname, 1);
     ebtablesRenameTmpRootChain(&buf, 1, ifname);
 
-    if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+    if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0)
         goto tear_down_tmpebchains;
 
     return 0;
@@ -3401,7 +3413,7 @@ ebtablesApplyDHCPOnlyRules(const char *i
     ebtablesRenameTmpRootChain(&buf, 1, ifname);
     ebtablesRenameTmpRootChain(&buf, 0, ifname);
 
-    if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+    if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0)
         goto tear_down_tmpebchains;
 
     VIR_FREE(srcIPParam);
@@ -3474,7 +3486,7 @@ ebtablesApplyDropAllRules(const char *if
     ebtablesRenameTmpRootChain(&buf, 1, ifname);
     ebtablesRenameTmpRootChain(&buf, 0, ifname);
 
-    if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+    if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0)
         goto tear_down_tmpebchains;
 
     return 0;
@@ -3517,7 +3529,7 @@ static int ebtablesCleanAll(const char *
     ebtablesRemoveTmpRootChain(&buf, 1, ifname);
     ebtablesRemoveTmpRootChain(&buf, 0, ifname);
 
-    ebiptablesExecCLI(&buf, &cli_status);
+    ebiptablesExecCLI(&buf, &cli_status, NULL);
     return 0;
 }
 
@@ -3677,6 +3689,7 @@ ebiptablesApplyNewRules(virConnectPtr co
     bool haveIp6tables = false;
     ebiptablesRuleInstPtr ebtChains = NULL;
     int nEbtChains = 0;
+    char *errmsg = NULL;
 
     if (!chains_in_set || !chains_out_set) {
         virReportOOMError();
@@ -3715,7 +3728,7 @@ ebiptablesApplyNewRules(virConnectPtr co
         ebtablesRemoveTmpSubChains(&buf, ifname);
         ebtablesRemoveTmpRootChain(&buf, 1, ifname);
         ebtablesRemoveTmpRootChain(&buf, 0, ifname);
-        ebiptablesExecCLI(&buf, &cli_status);
+        ebiptablesExecCLI(&buf, &cli_status, NULL);
     }
 
     /* create needed chains */
@@ -3730,7 +3743,7 @@ ebiptablesApplyNewRules(virConnectPtr co
         qsort(&ebtChains[0], nEbtChains, sizeof(ebtChains[0]),
               ebiptablesRuleOrderSort);
 
-    if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+    if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
         goto tear_down_tmpebchains;
 
     /* process ebtables commands; interleave commands from filters with
@@ -3764,7 +3777,7 @@ ebiptablesApplyNewRules(virConnectPtr co
                               ebtChains[j++].commandTemplate,
                               'A', -1, 1);
 
-    if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+    if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
         goto tear_down_tmpebchains;
 
     if (haveIptables) {
@@ -3773,17 +3786,17 @@ ebiptablesApplyNewRules(virConnectPtr co
 
         iptablesCreateBaseChains(iptables_cmd_path, &buf);
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
             goto tear_down_tmpebchains;
 
         iptablesCreateTmpRootChains(iptables_cmd_path, &buf, ifname);
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
            goto tear_down_tmpiptchains;
 
         iptablesLinkTmpRootChains(iptables_cmd_path, &buf, ifname);
         iptablesSetupVirtInPost(iptables_cmd_path, &buf, ifname);
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
            goto tear_down_tmpiptchains;
 
         for (i = 0; i < nruleInstances; i++) {
@@ -3794,7 +3807,7 @@ ebiptablesApplyNewRules(virConnectPtr co
                                     'A', -1, 1);
         }
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
            goto tear_down_tmpiptchains;
 
         iptablesCheckBridgeNFCallEnabled(false);
@@ -3806,17 +3819,17 @@ ebiptablesApplyNewRules(virConnectPtr co
 
         iptablesCreateBaseChains(ip6tables_cmd_path, &buf);
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
             goto tear_down_tmpiptchains;
 
         iptablesCreateTmpRootChains(ip6tables_cmd_path, &buf, ifname);
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
            goto tear_down_tmpip6tchains;
 
         iptablesLinkTmpRootChains(ip6tables_cmd_path, &buf, ifname);
         iptablesSetupVirtInPost(ip6tables_cmd_path, &buf, ifname);
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
            goto tear_down_tmpip6tchains;
 
         for (i = 0; i < nruleInstances; i++) {
@@ -3826,7 +3839,7 @@ ebiptablesApplyNewRules(virConnectPtr co
                                     'A', -1, 1);
         }
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+        if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
            goto tear_down_tmpip6tchains;
 
         iptablesCheckBridgeNFCallEnabled(true);
@@ -3837,7 +3850,7 @@ ebiptablesApplyNewRules(virConnectPtr co
     if (virHashSize(chains_out_set) != 0)
         ebtablesLinkTmpRootChain(&buf, 0, ifname, 1);
 
-    if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
+    if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0)
         goto tear_down_ebsubchains_and_unlink;
 
     virHashFree(chains_in_set);
@@ -3847,6 +3860,8 @@ ebiptablesApplyNewRules(virConnectPtr co
         VIR_FREE(ebtChains[i].commandTemplate);
     VIR_FREE(ebtChains);
 
+    VIR_FREE(errmsg);
+
     return 0;
 
 tear_down_ebsubchains_and_unlink:
@@ -3874,12 +3889,13 @@ tear_down_tmpebchains:
         ebtablesRemoveTmpRootChain(&buf, 0, ifname);
     }
 
-    ebiptablesExecCLI(&buf, &cli_status);
+    ebiptablesExecCLI(&buf, &cli_status, NULL);
 
     virNWFilterReportError(VIR_ERR_BUILD_FIREWALL,
                            _("Some rules could not be created for "
-                             "interface %s."),
-                           ifname);
+                             "interface %s : %s"),
+                           ifname,
+                           errmsg ? errmsg : "");
 
 exit_free_sets:
     virHashFree(chains_in_set);
@@ -3889,6 +3905,8 @@ exit_free_sets:
         VIR_FREE(ebtChains[i].commandTemplate);
     VIR_FREE(ebtChains);
 
+    VIR_FREE(errmsg);
+
     return 1;
 }
 
@@ -3919,7 +3937,7 @@ ebiptablesTearNewRules(virConnectPtr con
         ebtablesRemoveTmpRootChain(&buf, 0, ifname);
     }
 
-    ebiptablesExecCLI(&buf, &cli_status);
+    ebiptablesExecCLI(&buf, &cli_status, NULL);
 
     return 0;
 }
@@ -3938,7 +3956,7 @@ ebiptablesTearOldRules(virConnectPtr con
         iptablesRemoveRootChains(iptables_cmd_path, &buf, ifname);
 
         iptablesRenameTmpRootChains(iptables_cmd_path, &buf, ifname);
-        ebiptablesExecCLI(&buf, &cli_status);
+        ebiptablesExecCLI(&buf, &cli_status, NULL);
     }
 
     if (ip6tables_cmd_path) {
@@ -3946,7 +3964,7 @@ ebiptablesTearOldRules(virConnectPtr con
         iptablesRemoveRootChains(ip6tables_cmd_path, &buf, ifname);
 
         iptablesRenameTmpRootChains(ip6tables_cmd_path, &buf, ifname);
-        ebiptablesExecCLI(&buf, &cli_status);
+        ebiptablesExecCLI(&buf, &cli_status, NULL);
     }
 
     if (ebtables_cmd_path) {
@@ -3960,7 +3978,7 @@ ebiptablesTearOldRules(virConnectPtr con
 
         ebtablesRenameTmpSubAndRootChains(&buf, ifname);
 
-        ebiptablesExecCLI(&buf, &cli_status);
+        ebiptablesExecCLI(&buf, &cli_status, NULL);
     }
 
     return 0;
@@ -3997,7 +4015,7 @@ ebiptablesRemoveRules(virConnectPtr conn
                               'D', -1,
                               0);
 
-    if (ebiptablesExecCLI(&buf, &cli_status))
+    if (ebiptablesExecCLI(&buf, &cli_status, NULL))
         goto err_exit;
 
     if (cli_status) {
@@ -4048,7 +4066,7 @@ ebiptablesAllTeardown(const char *ifname
         ebtablesRemoveRootChain(&buf, 1, ifname);
         ebtablesRemoveRootChain(&buf, 0, ifname);
     }
-    ebiptablesExecCLI(&buf, &cli_status);
+    ebiptablesExecCLI(&buf, &cli_status, NULL);
 
     return 0;
 }
@@ -4103,7 +4121,7 @@ ebiptablesDriverInit(bool privileged)
                           ebtables_cmd_path, EBTABLES_DEFAULT_TABLE,
                           CMD_STOPONERR(1));
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status)
+        if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status)
              VIR_FREE(ebtables_cmd_path);
     }
 
@@ -4116,7 +4134,7 @@ ebiptablesDriverInit(bool privileged)
                           iptables_cmd_path,
                           CMD_STOPONERR(1));
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status)
+        if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status)
              VIR_FREE(iptables_cmd_path);
     }
 
@@ -4129,7 +4147,7 @@ ebiptablesDriverInit(bool privileged)
                           ip6tables_cmd_path,
                           CMD_STOPONERR(1));
 
-        if (ebiptablesExecCLI(&buf, &cli_status) || cli_status)
+        if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status)
              VIR_FREE(ip6tables_cmd_path);
     }
 

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