[libvirt] [PATCH v2 3/7] util: new virFirewallD APIs + docs

[Daniel P. Berrangé]

On Thu, Jan 31, 2019 at 08:24:54PM -0500, Laine Stump wrote:
> virFirewallDGetBackend() reports whether firewalld is currently using
> an iptables or an nftables backend.
> 
> virFirewallDGetVersion() learns the version of the firewalld running
> on this system and returns it as 1000000*major + 1000*minor + micro.
> 
> virFirewallDGetZones() gets a list of all currently active firewalld
> zones.
> 
> virFirewallDInterfaceSetZone() sets the firewalld zone of the given
> interface.
> 
> virFirewallDZoneExists() can be used to learn whether or not a
> particular zone is present and active in firewalld.
> 
> Signed-off-by: Laine Stump <laine at laine.org>
> ---


> +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.

> +
> +    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.

> +
> + 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 :-)

> +    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
-- 
|: 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 :|