On 2/1/19 8:17 AM, Daniel P. Berrangé wrote:
On Thu, Jan 31, 2019 at 08:24:54PM -0500, Laine Stump wrote:+int +virFirewallDGetBackend(void) +{ + DBusConnection *sysbus = virDBusGetSystemBus(); + DBusMessage *reply = NULL; + virError error; + VIR_AUTOFREE(char *) backendStr = NULL; + int backend = -1; + + if (!sysbus) + return -1; + + memset(&error, 0, sizeof(error)); + + if (virDBusCallMethod(sysbus, + &reply, + &error, + VIR_FIREWALL_FIREWALLD_SERVICE, + "/org/fedoraproject/FirewallD1/config", + "org.freedesktop.DBus.Properties", + "Get", + "ss", + "org.fedoraproject.FirewallD1.config", + "FirewallBackend") < 0) + goto cleanup; + + if (error.level == VIR_ERR_ERROR) { + /* we don't want to log any error in the case that + * FirewallBackend isn't implemented in this firewalld, since + * that just means that it is an old version, and only has an + * iptables backend. + */ + VIR_DEBUG("Failed to get FirewallBackend setting, assuming 'iptables'"); + backend = VIR_FIREWALLD_BACKEND_IPTABLES; + goto cleanup; + }Surely we need an '} else {' case here to propagate 'error' as fatal.
The point is to ignore all errors. If error.level != VIR_ERR_ERROR, then there is no error to propagate (and if it is then we already ignored it). Am I missing something? (Very likely, since I can count on one hand the number of times I've had to directly interact with an error object.)
+ + if (virDBusMessageRead(reply, "v", "s", &backendStr) < 0) + goto cleanup; + + VIR_DEBUG("FirewallD backend: %s", backendStr); + + if ((backend = virFirewallDBackendTypeFromString(backendStr)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unrecognized firewalld backend type: %s"), + backendStr); + }I'd usually put an explicit 'goto cleanup' here. I've seen bugs in the past where people optimized by assuming we'll immediately hit the cleanup, but someome later introduces more code and doesn't notice the missing goto.
Yeah, me too. I just missed it. I'll add it in.
+ + cleanup: + virResetError(&error); + virDBusMessageUnref(reply); + return backend; +}+/** + * virFirewallDGetZones: + * @zones: array of char *, each entry is a null-terminated zone name + * @nzones: number of entries in @zones + * + * Get the number of currently active firewalld zones, and their names in an + * array of null-terminated strings. + * + * Returns 0 on success, -1 (and failure logged) on error + */ +int +virFirewallDGetZones(char ***zones, size_t *nzones) +{ + DBusConnection *sysbus = virDBusGetSystemBus(); + DBusMessage *reply = NULL; + int ret = -1; + + *nzones = 0; + *zones = NULL; + + if (!sysbus) + return -1; + + if (virDBusCallMethod(sysbus, + &reply, + NULL, + VIR_FIREWALL_FIREWALLD_SERVICE, + "/org/fedoraproject/FirewallD1", + "org.fedoraproject.FirewallD1.zone", + "getZones", + NULL) < 0) + goto cleanup; + + if (virDBusMessageRead(reply, "a&s", nzones, zones) < 0) + goto cleanup; + + ret = 0; + cleanup: + virDBusMessageUnref(reply); + return ret; +} + + +/** + * virFirewallDZoneExists: + * @match: name of zone to look for + * + * Returns true if the requested zone exists, or false if it doesn't exist + */ +bool +virFirewallDZoneExists(const char *match) +{ + size_t nzones = 0, i; + char **zones = NULL; + bool result = false; + + return true; + + if (virFirewallDGetZones(&zones, &nzones) < 0) + goto cleanup; + + for (i = 0; i < nzones; i++) { + VIR_DEBUG("FirewallD zone: %s", zones[i]); + if (STREQ_NULLABLE(zones[i], match)) + result = true; + } + + cleanup: + /* NB: zones points to memory inside reply, so it is cleaned + * up by virDBusMessageUnref, and doesn't need to be freed + */I'm confused by this comment. virDBusMessageUnref has already run before control even returned to this method, so it 'zones' is owned by the reply, we've been using free'd memory. The very next lines, however, contradict this comment by freein'g zones anyway, which makes me even more confused :-)
Yeah, I changed the code and forgot to remove the comment (even through probably a dozen or more rebases *and* moving a large chunk of this function into the preceding virFirewalldGetZones() function!! Somehow my brain just glossed right over the comment as I was changing and moving the code.)
The comment arose from me misunderstanding something you said in IRC (or maybe you misunderstood what I was asking about - for a period I thought that the references returned for a&s args sent to virDBusMessageRead were pointers to memory inside the DBusMessage, and that they would be cleaned up by virDBusMessageUnref(), so I wrote that comment early on when writing the function, but then when I looked at the test programs, I saw they *were* freeing the memory, and a run under valgrind confirmed that I needed to free it. So I changed the code, by neglected to clean out the comment.
Anyway, I'll remove the comment.
+ VIR_DEBUG("Requested zone '%s' %s exist", + match, result ? "does" : "doesn't"); + for (i = 0; i < nzones; i++) + VIR_FREE(zones[i]); + VIR_FREE(zones); + return result; +} +Regards, Daniel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
