Re: [Qemu-devel] [PATCH 1/2] bridge helper: support conf dirs

[Corey Bryant]

This series seems reasonable to me.  I just have some comments on 
implementation details.

On 02/24/2013 09:00 PM, Doug Goldstein wrote:
> Allow the bridge helper to take a config directory rather than having to
> specify every file in the directory manually via an include statement.
>
> Signed-off-by: Doug Goldstein <address@hidden>
> CC: Anthony Liguori <address@hidden>
> CC: Richa Marwaha <address@hidden>
> CC: Corey Bryant <address@hidden>
> TO: address@hidden
> ---
>   qemu-bridge-helper.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index 287bfd5..b8771a3 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -16,6 +16,7 @@
>   #include "config-host.h"
>
>   #include <stdio.h>
> +#include <dirent.h>
>   #include <errno.h>
>   #include <fcntl.h>
>   #include <unistd.h>
> @@ -70,6 +71,26 @@ static void usage(void)
>               "Usage: qemu-bridge-helper [--use-vnet] --br=bridge 
> --fd=unixfd\n");
>   }
>
> +static int filter_bridge_conf_dir(const struct dirent *entry)
> +{
> +    ssize_t name_pos;
> +
> +    /* We want to check the last 5 bytes for '.conf' */
> +    name_pos = strlen(entry->d_name) - 6;

a.conf sets name_pos=0..

> +
> +    /* We need the file to at least be called 'a.conf' to make
> +     * sense of this.
> +     */
> +    if (name_pos < 1)
> +        return 0;

..so if the name is a.conf we'd return 0 here which means it would be 
skipped.  That's not right.

> +
> +    /* If the file didn't end in '.conf', skip it */
> +    if (strcmp(".conf", entry->d_name + name_pos))
> +        return 0;
> +
> +    return 1;
> +}
> +

I think you can simplify this function to something like:

static int is_filter_bridge_conf_dir(const struct dirent *entry)
{
    size_t len = strlen(entry->d_name);

    if (len > 5 &&
        strcmp(".conf", &entry->d_name[len-5]) == 0) {
        return 1;
    }

    return 0;
}

>   static int parse_acl_file(const char *filename, ACLList *acl_list)
>   {
>       FILE *f;
> @@ -84,6 +105,9 @@ static int parse_acl_file(const char *filename, ACLList 
> *acl_list)
>       while (fgets(line, sizeof(line), f) != NULL) {
>           char *ptr = line;
>           char *cmd, *arg, *argend;
> +        struct dirent **include_list = NULL;
> +        int i, include_count;
> +        char *conf_file;
>
>           while (isspace(*ptr)) {
>               ptr++;
> @@ -137,6 +161,37 @@ static int parse_acl_file(const char *filename, ACLList 
> *acl_list)
>                   snprintf(acl_rule->iface, IFNAMSIZ, "%s", arg);
>               }
>               QSIMPLEQ_INSERT_TAIL(acl_list, acl_rule, entry);
> +        } else if (strcmp(cmd, "includedir") == 0) {
> +            include_count = scandir(arg, &include_list,
> +                                    filter_bridge_conf_dir, NULL);
> +            if (include_count < 0) {
> +                fprintf(stderr, "Unable to retrieve conf files from '%s': 
> %s\n",
> +                        arg, strerror(errno));
> +                fclose(f);
> +                return -1;
> +            }
> +
> +            for (i = 0; i < include_count; i++) {
> +                if (asprintf(&conf_file, "%s/%s", arg,
> +                             include_list[i]->d_name) < 0) {
> +                    fprintf(stderr, "Failed to allocate memory for "
> +                            "file path: %s/%s\n",
> +                            arg, include_list[i]->d_name);
> +                    fclose(f);
> +                    errno = ENOMEM;
> +                    return -1;

The new failure paths in this patch have memory leaks that are fixed in 
patch 2.  It might be simpler if you just merge patch 1 and 2.

> +                }
> +
> +                parse_acl_file(conf_file, acl_list);
> +
> +                free(conf_file);
> +                free(include_list[i]);
> +                include_list[i] = NULL;
> +            }
> +            free(include_list);
> +            include_list = NULL;
> +            include_count = 0;
> +
>           } else if (strcmp(cmd, "include") == 0) {
>               /* ignore errors */
>               parse_acl_file(arg, acl_list);

--
Regards,
Corey Bryant