9:06 a.m.
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