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

[Laine Stump]

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