[libvirt] Resubmission #2: [PATCH 2/3] sVirt AppArmor security driver

Daniel P. Berrange berrange at redhat.com
Wed Sep 30 10:16:11 UTC 2009


On Fri, Sep 25, 2009 at 05:49:01PM -0500, Jamie Strandboge wrote:
> diff -Naurp libvirt.orig/src/security/security_apparmor.c libvirt/src/security/security_apparmor.c
> --- libvirt.orig/src/security/security_apparmor.c	1969-12-31 18:00:00.000000000 -0600
> +++ libvirt/src/security/security_apparmor.c	2009-09-25 17:00:13.000000000 -0500
> +/*
> + * profile_status returns '-1' on error, '0' if loaded
> + *
> + * If check_enforcing is set to '1', then returns '-1' on error, '0' if
> + * loaded in complain mode, and '1' if loaded in enforcing mode.
> + */
> +static int
> +profile_status(const char *str, const int check_enforcing)
> +{
> +    char *content = NULL;
> +    char *tmp = NULL;
> +    char *etmp = NULL;
> +    int rc = -1;
> +
> +    /* create string that is '<str> \0' for accurate matching */
> +    if (virAsprintf(&tmp, "%s ", str) == -1)
> +        return rc;
> +
> +    if (check_enforcing != 0) {
> +        /* create string that is '<str> (enforce)\0' for accurate matching */
> +        if (virAsprintf(&etmp, "%s (enforce)", str) == -1) {
> +            VIR_FREE(tmp);
> +            return rc;
> +        }
> +    }
> +
> +    if (virFileReadAll(APPARMOR_PROFILES_PATH, MAX_FILE_LEN, &content) < 0) {
> +        virReportSystemError(NULL, errno,
> +                             _("Failed to read AppArmor profiles list "
> +                             "\'%s\'"), APPARMOR_PROFILES_PATH);
> +        if (check_enforcing != 0)
> +            VIR_FREE(etmp);
> +        VIR_FREE(tmp);
> +        return rc;
> +    }
> +
> +    if (strstr(content, tmp) != NULL)
> +        rc = 0;
> +    if (check_enforcing != 0) {
> +        if (rc == 0 && strstr(content, etmp) != NULL)
> +            rc = 1;                 /* return '1' if loaded and enforcing */
> +        VIR_FREE(etmp);
> +    }

Since you've already got yourself a 'rc' variable declared & initialized,
it'd benice to replace the duplicated error paths:

   VIR_FREE(tmp)
   return rc;

With just a 'goto cleanup', and put a cleanup: label just at the endd
here:

> +    VIR_FREE(tmp);
> +    VIR_FREE(content);
> +
> +    return rc;
> +}
> +

> +/*
> + * profile_status_file returns '-1' on error, '0' if file on disk is in
> + * complain mode and '1' if file on disk is in enforcing mode
> + */
> +static int
> +profile_status_file(const char *str)
> +{
> +    char profile[PATH_MAX];
> +    char *content = NULL;
> +    char *tmp = NULL;
> +    int rc = -1;
> +    int len;
> +
> +    if (snprintf(profile, PATH_MAX, "%s/%s", APPARMOR_DIR "/libvirt", str)
> +       > PATH_MAX - 1) {
> +        virSecurityReportError(NULL, VIR_ERR_ERROR,
> +                               "%s", _("profile name exceeds maximum length"));
> +    }
> +
> +    if (!virFileExists(profile)) {
> +        return rc;
> +    }
> +
> +    if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) {
> +        virReportSystemError(NULL, errno,
> +                             _("Failed to read \'%s\'"), profile);
> +        return rc;
> +    }
> +
> +    /* create string that is ' <str> flags=(complain)\0' */
> +    if (virAsprintf(&tmp, " %s flags=(complain)", str) == -1) {
> +        virSecurityReportError(NULL, VIR_ERR_ERROR,
> +                               "%s", _("could not allocate memory"));

This one should just be   virReportOOMError(NULL);

> +        VIR_FREE(content);
> +        return rc;
> +    }
> +
> +    if (strstr(content, tmp) != NULL)
> +        rc = 0;
> +    else
> +        rc = 1;
> +
> +    VIR_FREE(tmp);
> +    VIR_FREE(content);
> +
> +    return rc;

Similar thought here about removing the duplicated cleanup paths in
favour of just jumping to the end of the method & having a single
cleanup code block

> +static int
> +load_profile(virConnectPtr conn, const char *profile, virDomainObjPtr vm,
> +             virDomainDiskDefPtr disk)
> +{
> +    const char *argv[7];
> +    int rc = -1, status;
> +    bool create = true;
> +    char *xml = NULL;
> +    int pipefd[2];
> +    pid_t child;
> +
> +    if (pipe(pipefd) < -1) {
> +        virReportSystemError(conn, errno, "%s", _("unable to create pipe"));
> +        return rc;
> +    }
> +
> +    xml = virDomainDefFormat(conn, vm->def, VIR_DOMAIN_XML_SECURE);
> +    if (!xml) {
> +        virSecurityReportError(conn, VIR_ERR_ERROR,
> +                               "%s", _("could not format XML"));
> +        goto failed;
> +    }

No need to report an error when the virDomainDef* functions
fail, since they'll have already done that for you.

> +
> +    if (profile_status_file(profile) >= 0)
> +        create = false;
> +
> +    argv[0] = VIRT_AA_HELPER_PATH;
> +    argv[2] = (char *) "-u";
> +    argv[3] = profile;
> +    argv[4] = NULL;
> +    if (create)
> +        argv[1] = (char *) "-c";
> +    else {
> +        argv[1] = (char *) "-r";
> +        if (disk && disk->src) {
> +            argv[4] = "-f";
> +            argv[5] = disk->src;
> +            argv[6] = NULL;
> +        }
> +    }
>
> +    if (virExec(conn, argv, NULL, NULL, &child,
> +                pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS) < 0)
> +        goto clean;

Casting away const multiple times here. THis can be avoided by
changing 

  const char *argv[7];

to be

  const char * const argv[7];

I find the out-of-order initialization of argv indicies here
a little confusing. Even though it'd be duplicating a little,
I think it'd be clearer to have

     if (create) {
        const char *const argv[] = {
           VIR_AA_HELPER_PATH, "-c", "-u", profile, NULL
        };
        ret = virExec(conn, argv, NULL, NULL, &child,
                      pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS);
     } else if (disk && disk->src) {
        const char *const argv[] = {
           VIR_AA_HELPER_PATH, "-r", "-u", profile, "-f", disk->src, NULL
        };
        ret = virExec(conn, argv, NULL, NULL, &child,
                      pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS);        
     } else {
        const char *const argv[] = {
           VIR_AA_HELPER_PATH, "-r", "-u", profile, NULL
        };
        ret = virExec(conn, argv, NULL, NULL, &child,
                      pipefd[0], NULL, NULL, VIR_EXEC_CLEAR_CAPS);
     }
     if (ret < 0)
       goto cleanup;

> +
> +
> +    /* parent continues here */
> +    if (safewrite(pipefd[1], xml, strlen(xml)) < 0) {
> +        virReportSystemError(conn, errno, "%s", _("unable to write to pipe"));
> +        goto clean;
> +    }
> +    close(pipefd[1]);
> +    rc = 0;
> +
> +  rewait:
> +    if (waitpid(child, &status, 0) != child) {
> +        if (errno == EINTR)
> +            goto rewait;
> +
> +        virSecurityReportError(conn, VIR_ERR_ERROR,
> +                               _("Unexpected exit status from virt-aa-helper "
> +                               "%d pid %lu"),
> +                               WEXITSTATUS(status), (unsigned long)child);
> +        rc = -1;
> +    }
> +
> +  clean:
> +    VIR_FREE(xml);
> +
> +  failed:
> +    if (pipefd[0] > 0)
> +        close(pipefd[0]);
> +    if (pipefd[1] > 0)
> +        close(pipefd[1]);
> +
> +    return rc;
> +}

> +
> +static int
> +remove_profile(const char *profile)
> +{
> +    const char *argv[5];
> +    int rc = -1;
> +
> +    argv[0] = VIRT_AA_HELPER_PATH;
> +    argv[1] = (char *) "-R";
> +    argv[2] = (char *) "-u";
> +    argv[3] = profile;
> +    argv[4] = NULL;

To preserve const-ness & remove hardcoded array indexes, this
could be written as 

    const char * const argv[] = {
       VIRT_AA_HELPER_PATH, "-R", "-u", profile, NULL
    };

> +
> +    if (virRun(NULL, argv, NULL) == 0)
> +        rc = 0;
> +
> +    return rc;
> +}
> +
> +/*
> + * profile_name is buffer to hold name and len is how many bytes in the
> + * buffer
> + */
> +static int
> +get_profile_name(virConnectPtr conn, virDomainObjPtr vm,
> +                 char *profile_name, const size_t len)
> +{
> +    virDomainPtr dom = NULL;
> +    int rc = -1;
> +
> +    if (len < PROFILE_NAME_SIZE) {
> +        virSecurityReportError(conn, VIR_ERR_ERROR,
> +                               "%s", _("profile_name has wrong size"));
> +        return rc;
> +    }
> +    profile_name[0] = '\0';
> +    strcat(profile_name, AA_PREFIX);
> +
> +    /* generate the profile name */
> +    dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> +    dom->id = vm->def->id;
> +
> +    if (virDomainGetUUIDString(dom,
> +        &profile_name[strlen(profile_name)]) != 0) {
> +        virSecurityReportError(conn, VIR_ERR_ERROR,
> +                               "%s", _("could not find uuid for VM"));
> +        return rc;
> +    }
> +
> +    return 0;
> +}

I'm not really following why this code needs to call out to the public
API just to get the UUID into string format. It not clear that this is
sufficiently checking the allocate char * is large enough for the UUID
string result.

I think I'd prefer to see this returning a string allocated on the heap,
rather than the caller's stack. The whole method could thus be simplified
down to just

     char *uuidstr[VIR_UUID_STRING_BUFLEN];
     char *ret;
     virUUIDFormat(vm->def->uuid, uuidstr);

     if (virAsprintf(&ret, "%s%s", AA_PREFIX, uuidstr) < 0)
        return NULL;
     return ret;

> +/* returns -1 on error or profile for libvirtd is unconfined, 0 if complain
> + * mode and 1 if enforcing
> + */
> +static int
> +use_apparmor(void)
> +{
> +    char libvirt_daemon[PATH_MAX];
> +    int rc = -1;
> +    ssize_t len = 0;
> +
> +    if ((len = readlink("/proc/self/exe", libvirt_daemon,
> +                        PATH_MAX - 1)) < 0) {
> +        virSecurityReportError(NULL, VIR_ERR_ERROR,
> +                               "%s", _("could not find libvirtd"));
> +        return rc;
> +    }
> +    libvirt_daemon[len] = '\0';
> +
> +    if (access(APPARMOR_PROFILES_PATH, R_OK) != 0)
> +        return rc;
> +
> +    return profile_status(libvirt_daemon, 1);
> +}

Is this the best way to check if app armour is enabled on a  host ?
It ought to be possible to use the app armour security driver for
protecting guests regardless of whether the libvirtd daemon itself
has a profile surely.

> +static int
> +AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
> +{
> +    int rc = -1;
> +    char profile_name[PROFILE_NAME_SIZE];
> +
> +    profile_name[0] = '\0';
> +
> +    if ((vm->def->seclabel.label) ||
> +        (vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) {
> +        virSecurityReportError(conn, VIR_ERR_ERROR,
> +                               "%s",
> +                               _("security label already defined for VM"));
> +        return rc;
> +    }
> +
> +    if (get_profile_name(conn, vm, profile_name, sizeof(profile_name)) < 0)
> +        return rc;
> +
> +    /* if the profile is not already loaded, then load one */
> +    if (profile_loaded(profile_name) < 0) {
> +        if (load_profile(conn, profile_name, vm, NULL) < 0) {
> +            virSecurityReportError(conn, VIR_ERR_ERROR,
> +                                   _("cannot generate AppArmor profile "
> +                                   "\'%s\'"), profile_name);
> +            return rc;
> +        }
> +    }
> +
> +    vm->def->seclabel.label = strndup(profile_name, strlen(profile_name));
> +    if (!vm->def->seclabel.label) {
> +        virSecurityReportError(conn, VIR_ERR_ERROR,
> +                               "%s", _("cannot generate AppArmor profile "
> +                               "name (label)"));
> +        goto err;
> +    }
> +
> +    /* set imagelabel the same as label (but we won't use it) */
> +    vm->def->seclabel.imagelabel = strndup(profile_name,
> +                                           strlen(profile_name));
> +    if (!vm->def->seclabel.imagelabel) {
> +        virSecurityReportError(conn, VIR_ERR_ERROR,
> +                               "%s",
> +                               _("cannot generate AppArmor profile name "
> +                               "(imagelabel)"));
> +        goto err;
> +    }

These two strndup failures should also be  virReportOOMError()

> +
> +    vm->def->seclabel.model = strdup(SECURITY_APPARMOR_NAME);
> +    if (!vm->def->seclabel.model) {
> +        virReportOOMError(conn);
> +        goto err;
> +    }
> +
> +    rc = 0;
> +    goto done;
> +
> +  err:
> +    remove_profile(profile_name);
> +    VIR_FREE(vm->def->seclabel.label);
> +    VIR_FREE(vm->def->seclabel.imagelabel);
> +    VIR_FREE(vm->def->seclabel.model);
> +  done:
> +    return rc;
> +}
> +

> diff -Naurp libvirt.orig/tools/Makefile.am libvirt/tools/Makefile.am
> --- libvirt.orig/tools/Makefile.am	2009-09-22 12:51:57.000000000 -0500
> +++ libvirt/tools/Makefile.am	2009-09-25 17:00:13.000000000 -0500
> @@ -51,6 +51,33 @@ virsh_CFLAGS = 							\
>  		$(COVERAGE_CFLAGS)				\
>  		$(LIBXML_CFLAGS)				\
>  		$(READLINE_CFLAGS)
> +
> +if WITH_SECDRIVER_APPARMOR
> +bin_PROGRAMS += virt-aa-helper

Since this program is only really intended for libvirt to call
rather than general admin usage I think it'd be beter to list
it as    libexec_PROGRAMS, so it ends up in /usr/libexec
instead of /usr/bin

I think I'd actually have it in the src/security/ directory,
since we usually keep libexec programs alongside the driver
code that's using them, just have tools/ for end user programs

> +/*
> + * run an apparmor_parser command
> + */
> +static int
> +parserCommand(const char *profile_name, const char cmd)
> +{
> +    const char *argv[4];
> +    char flag[3];
> +    char profile[PATH_MAX];
> +
> +    if (strchr("arR", cmd) == NULL) {
> +        vah_error(NULL, 0, "invalid flag");
> +        return -1;
> +    }
> +
> +    snprintf(flag, 3, "-%c", cmd);
> +
> +    if (snprintf(profile, PATH_MAX, "%s/%s",
> +                 APPARMOR_DIR "/libvirt", profile_name) > PATH_MAX - 1) {
> +        vah_error(NULL, 0, "profile name exceeds maximum length");
> +        return -1;
> +    }
> +
> +    if (!virFileExists(profile)) {
> +        vah_error(NULL, 0, "profile does not exist");
> +        return -1;
> +    }
> +
> +    argv[0] = (char *) "/sbin/apparmor_parser";
> +    argv[1] = flag;
> +    argv[2] = profile;
> +    argv[3] = NULL;

Same issue with const-ness  as earlier examples

> +
> +    if (virRun(NULL, argv, NULL) != 0) {
> +        vah_error(NULL, 0, "failed to run apparmor_parser");
> +        return -1;
> +    }
> +
> +    return 0;
> +}


> +static int
> +valid_uuid(const char *uuid)
> +{
> +    int i;
> +
> +    if (strlen(uuid) != PROFILE_NAME_SIZE - 1)
> +        return -1;
> +
> +    if (STRNEQLEN(AA_PREFIX, uuid, strlen(AA_PREFIX)))
> +        return -1;

This one can just be

      if (!STRPREFIX(uuid, AA_PREFIX))
            return -1;

> +
> +    for (i = strlen(AA_PREFIX); i < PROFILE_NAME_SIZE - 1; i++) {
> +        if (uuid[i] == '-')
> +            continue;
> +        if (!c_isxdigit(uuid[i]))
> +            return -1;
> +    }
> +    return 0;
> +}

How about just using  virUUIDParse to check UUID validity, such as

      char rawuuid[VIR_UUID_BUFLEN];
      if (virUUIDParse(uuid + strlen(AA_PREFIX), rawuuid) < 0)
          return -1;
      return 0;

> +
> +static int
> +valid_name(const char *name)
> +{
> +    /* just try to filter out any dangerous characters in the name that can be
> +     * used to subvert the profile */
> +    char *bad = (char *) " /[]*";

Please declare it const, rather than casting away const.

> +    int i;
> +
> +    if (strlen(name) == 0 || strlen(name) > PATH_MAX - 1)
> +        return -1;
> +
> +    for (i = 0; i < strlen(bad); i++)
> +        if (strchr(name, bad[i]) != NULL)
> +            return -1;

FYI, there'a convenient libc function for checking a string for bad
characters - strspn, you can use it in this way:

     if (strspn(name, bad) != strlen(name))
         return -1;

> +static int
> +valid_path(const char *path, const bool readonly)
> +{
> +    struct stat sb;
> +    int i;
> +    int npaths = 20;
> +    char **restricted;
> +    int rc = -1;
> +
> +    if (path == NULL || strlen(path) > PATH_MAX - 1) {
> +        vah_error(NULL, 0, "bad pathname");
> +        return rc;
> +    }
> +
> +    /* Don't allow double quotes, since we use them to quote the filename
> +     * and this will confuse the apparmor parser.
> +     */
> +    if (strchr(path, '"') != NULL)
> +        return 1;
> +
> +    if (VIR_ALLOC_N(restricted, npaths) < 0) {
> +        vah_error(NULL, 0, "could not allocate memory for paths");
> +        return rc;
> +    }
> +
> +    restricted[0] = (char *) "/bin/";
> +    restricted[1] = (char *) "/etc/";
> +    restricted[2] = (char *) "/lib";
> +    restricted[3] = (char *) "/lost+found/";
> +    restricted[4] = (char *) "/proc/";
> +    restricted[5] = (char *) "/sbin/";
> +    restricted[6] = (char *) "/selinux/";
> +    restricted[7] = (char *) "/sys/";
> +    restricted[8] = (char *) "/usr/bin/";
> +    restricted[9] = (char *) "/usr/lib";
> +    restricted[10] = (char *) "/usr/sbin/";
> +    restricted[11] = (char *) "/usr/share/";
> +    restricted[12] = (char *) "/usr/local/bin/";
> +    restricted[13] = (char *) "/usr/local/etc/";
> +    restricted[14] = (char *) "/usr/local/lib";
> +    restricted[15] = (char *) "/usr/local/sbin/";
> +    /* these paths are ok for readonly */
> +    restricted[16] = (char *) "/boot/";
> +    restricted[17] = (char *) "/vmlinuz";
> +    restricted[18] = (char *) "/initrd";
> +    restricted[19] = (char *) "/initrd.img";
> +
> +    /* don't check paths ok for readonly */
> +    if (readonly)
> +        npaths = 16;

More const issues. Rather than manually declaring the array
size, and indexing manually its safer to initialize the
array at time of declaration with all the strings.  I'd
probably go for separate arrays for the read-write vs
read-only difference

> +
> +    if (!virFileExists(path))
> +        vah_warning("path does not exist, skipping file type checks");
> +    else {
> +        if (stat(path, &sb) == -1)
> +            goto end;
> +
> +        rc = 1;
> +        switch (sb.st_mode & S_IFMT) {
> +            case S_IFDIR:
> +                goto end;
> +                break;
> +            case S_IFIFO:
> +                goto end;
> +                break;
> +            case S_IFSOCK:
> +                goto end;
> +                break;
> +            default:
> +                break;
> +        }
> +    }
> +
> +    for (i = 0; i < npaths; i++) {
> +        if (strlen(path) < strlen(restricted[i]))
> +            continue;
> +
> +        if (STREQLEN(path, restricted[i], strlen(restricted[i]))) {
> +            rc = 1;
> +            goto end;
> +        }
> +    }
> +    rc = 0;
> +
> +  end:
> +    VIR_FREE(restricted);
> +    return rc;
> +}

> +
> +
> +static int
> +vah_add_file(virBufferPtr buf, const char *path, const char *perms)
> +{
> +    char *tmp = NULL;
> +    int rc = -1;
> +    bool readonly = true;
> +
> +    if (path == NULL)
> +        return rc;
> +
> +    if (virFileExists(path)) {
> +        if ((tmp = realpath(path, NULL)) == NULL) {
> +            vah_error(NULL, 0, path);
> +            vah_error(NULL, 0, "  could not find realpath for disk");
> +            return rc;
> +        }
> +    } else
> +        if ((tmp = strdup(path)) == NULL)
> +            return rc;
> +
> +    if (strchr(perms, 'w') != NULL)
> +        readonly = false;
> +
> +    rc = valid_path(tmp, readonly);
> +    if (rc != 0) {
> +        if (rc > 0) {
> +            vah_error(NULL, 0, path);
> +            vah_error(NULL, 0, "  skipped restricted file");
> +        }
> +        goto clean;
> +    }
> +
> +    virBufferVSprintf(buf, "  \"%s\" %s,\n", tmp, perms);
> +
> +    if (virBufferError(buf)) {
> +         vah_error(NULL, 0, "failed to allocate file buffer");
> +         rc = -1;
> +    }

It is not really neeccesssar to check virBufferError here. The
idea is that you can call virBufferVSprintf() as many times
as you like in a row without any checking, and then only check
virBufferError at the end when you've finishing writing to the
buffer.

> +
> +  clean:
> +    free(tmp);
> +
> +    return rc;
> +}
> +
> +static int
> +get_files(vahControl * ctl)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    virBuffer uuid_buf = VIR_BUFFER_INITIALIZER;
> +    int i;
> +    int rc = -1;
> +    char *uuid;
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +    /* verify uuid is same as what we were given on the command line */
> +    virUUIDFormat(ctl->def->uuid, uuidstr);
> +    virBufferVSprintf(&uuid_buf, "%s%s", AA_PREFIX, uuidstr);
> +    if (virBufferError(&uuid_buf)) {
> +        vah_error(ctl, 0, "failed to allocate file buffer");
> +        return rc;
> +    }
> +    uuid = virBufferContentAndReset(&uuid_buf);

Using virBuffer for a single printf here is a little overkill - the
virAsprintf() function would be more than sufficient

> +    if (STRNEQ(uuid, ctl->uuid)) {
> +        vah_error(ctl, 0, "given uuid does not match XML uuid");
> +        goto clean;
> +    }
> +
> +    for (i = 0; i < ctl->def->ndisks; i++)
> +        if (ctl->def->disks[i] && ctl->def->disks[i]->src)
> +            if (vah_add_file(&buf, ctl->def->disks[i]->src, "rw") != 0)
> +                goto clean;
> +
> +    for (i = 0; i < ctl->def->nserials; i++)
> +        if (ctl->def->serials[i] && ctl->def->serials[i]->data.file.path)
> +            if (vah_add_file(&buf,
> +                             ctl->def->serials[i]->data.file.path, "w") != 0)
> +                goto clean;
> +
> +    if (ctl->def->console && ctl->def->console->data.file.path)
> +        if (vah_add_file(&buf, ctl->def->console->data.file.path, "w") != 0)
> +            goto clean;
> +
> +    if (ctl->def->os.kernel && ctl->def->os.kernel)
> +        if (vah_add_file(&buf, ctl->def->os.kernel, "r") != 0)
> +            goto clean;
> +
> +    if (ctl->def->os.initrd && ctl->def->os.initrd)
> +        if (vah_add_file(&buf, ctl->def->os.initrd, "r") != 0)
> +            goto clean;
> +
> +    if (ctl->def->os.loader && ctl->def->os.loader)
> +        if (vah_add_file(&buf, ctl->def->os.loader, "r") != 0)
> +            goto clean;
> +
> +    /* needs patch to hostusb.h to work
> +    for (i = 0; i < ctl->def->nhostdevs; i++)
> +        if (ctl->def->hostdevs[i]) {
> +            virDomainHostdevDefPtr hostdev = ctl->def->hostdevs[i];
> +            if (hostdev->source.subsys.type ==
> +                VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
> +                usbDevice *usb = usbGetDevice(NULL,
> +                           hostdev->source.subsys.u.usb.bus,
> +                           hostdev->source.subsys.u.usb.device);
> +                if (usb == NULL)
> +                    continue;
> +                rc = vah_add_file(&buf, usb->path, "rw");
> +                usbFreeDevice(NULL, usb);

This is the bit of code that should be changed to use the
usbDeviceFileIterate() method, since you can't assume there
is only 1 path which is why we didn't make 'path' public.

> +                if (rc != 0)
> +                    goto clean;
> +            }
> +        }
> +    */
> +
> +    if (ctl->newdisk)
> +        if (vah_add_file(&buf, ctl->newdisk, "rw") != 0)
> +            goto clean;

If you remove the virBufferError check from the vah_add_file
calls, it could just go in here
> +
> +    rc = 0;
> +    ctl->files = virBufferContentAndReset(&buf);
> +  clean:
> +    VIR_FREE(uuid);
> +    return rc;
> +}
> +


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|




More information about the libvir-list mailing list