On Wed, Jan 09, 2019 at 09:57:34PM -0500, Laine Stump wrote:
Since I'm going to be adding at least one more firewalld-specific function, this seems like a good time to separate the code that's unique to firewalld from the more-generic "firewall" file.
Signed-off-by: Laine Stump <laine@laine.org> --- include/libvirt/virterror.h | 1 + src/libvirt_private.syms | 3 + src/util/Makefile.inc.am | 2 + src/util/virerror.c | 1 + src/util/virfirewall.c | 86 ++---------------------- src/util/virfirewalld.c | 128 ++++++++++++++++++++++++++++++++++++ src/util/virfirewalld.h | 33 ++++++++++ src/util/virfirewallpriv.h | 2 - tests/virfirewalltest.c | 1 + 9 files changed, 173 insertions(+), 84 deletions(-) create mode 100644 src/util/virfirewalld.c create mode 100644 src/util/virfirewalld.h
+ if (error.level == VIR_ERR_ERROR) { + /* + * As of firewalld-0.3.9.3-1.fc20.noarch the name and + * message fields in the error look like + * + * name="org.freedesktop.DBus.Python.dbus.exceptions.DBusException" + * message="COMMAND_FAILED: '/sbin/iptables --table filter --delete + * INPUT --in-interface virbr0 --protocol udp --destination-port 53 + * --jump ACCEPT' failed: iptables: Bad rule (does a matching rule + * exist in that chain?)." + * + * We'd like to only ignore DBus errors precisely related to the failure + * of iptables/ebtables commands. A well designed DBus interface would + * return specific named exceptions not the top level generic python dbus + * exception name. With this current scheme our only option is todo a + * sub-string match for 'COMMAND_FAILED' on the message. eg like + * + * if (ignoreErrors && + * STREQ(error.name, + * "org.freedesktop.DBus.Python.dbus.exceptions.DBusException") && + * STRPREFIX(error.message, "COMMAND_FAILED")) + * ... + * + * But this risks our error detecting code being broken if firewalld changes + * ever alter the message string, so we're avoiding doing that. + */
We really ought to ask the firewalld guys to improve this since we have good communication with them now :-)
+ if (ignoreErrors) { + VIR_DEBUG("Ignoring error '%s': '%s'", + error.str1, error.message); + } else { + virReportErrorObject(&error); + goto cleanup; + } + } else { + if (virDBusMessageRead(reply, "s", output) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virResetError(&error); + virDBusMessageUnref(reply); + return ret; +}
diff --git a/src/util/virfirewalld.h b/src/util/virfirewalld.h new file mode 100644 index 0000000000..c1c929399a --- /dev/null +++ b/src/util/virfirewalld.h @@ -0,0 +1,33 @@
+#ifndef LIBVIRT_VIRFIREWALLD_H +# define LIBVIRT_VIRFIREWALLD_H + +# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1"
This is only needed for tests so should go in a priv.h header as it was before.
+ +int virFirewallDStatus(void);
Perhaps virFirewallDIsRegistered() ? Could you put API docs for this since it has an unusal tri-state return value.
+ +int virFirewallDApplyRule(virFirewallLayer layer, + char **args, size_t argsLen, + bool ignoreErrors, + char **output);
Would be nice to have API docs for this too
+ +#endif /* LIBVIRT_VIRFIREWALLD_H */ diff --git a/src/util/virfirewallpriv.h b/src/util/virfirewallpriv.h index efa94a7da4..7c31d0680d 100644 --- a/src/util/virfirewallpriv.h +++ b/src/util/virfirewallpriv.h @@ -27,8 +27,6 @@
# include "virfirewall.h"
-# define VIR_FIREWALL_FIREWALLD_SERVICE "org.fedoraproject.FirewallD1" - typedef enum { VIR_FIREWALL_BACKEND_AUTOMATIC, VIR_FIREWALL_BACKEND_DIRECT, diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 63b9ced820..573ab1f9cd 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -27,6 +27,7 @@ # include "vircommandpriv.h" # define LIBVIRT_VIRFIREWALLPRIV_H_ALLOW # include "virfirewallpriv.h" +# include "virfirewalld.h"
Should be virfirewalldpriv.h Assuming those small changes are made Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|