On Thu, May 28, 2009 at 09:04:55AM -0500, Doug Goldstein wrote:

Several distros out there install "kvm-img" when they install KVM and do not install "qemu-img". The following patch checks for kvm-img and qemu-img and does the right thing. I've taken it a step further and made the check a run-time check instead of a build-time check since people are able to remove and install KVM, QEMU and Xen components as they please and as such may invalidate the build-time check. The patch prefers "kvm-img" over "qemu-img" over "qcow-create" since there was a comment that libvirt wanted to support a feature of qemu-img that wasn't present in upstream qemu releases, however the feature is present in kvm-img.

The patch adds a new util function called "virFindFileInPath()" which merely searches PATH for the actual location of the binary. It might be worth-while to make some of the other build-time tool checks run-time with that function since it would allow users to get more features just by installing new tools and wouldn't require distros to disable features because they don't want libvirt to be too dependency heavy.

[snip]

diff -Nur libvirt-0.6.3/src/storage_backend_fs.c libvirt-0.6.3-kvm-img/src/storage_backend_fs.c --- libvirt-0.6.3/src/storage_backend_fs.c 2009-04-17 14:07:48.000000000 -0500 +++ libvirt-0.6.3-kvm-img/src/storage_backend_fs.c 2009-05-27 17:13:27.000000000 -0500 @@ -1013,7 +1013,7 @@

/** * Allocate a new file as a volume. This is either done directly - * for raw/sparse files, or by calling qemu-img/qcow-create for + * for raw/sparse files, or by calling kvm-img/qemu-img/qcow-create for * special kinds of files */ static int @@ -1021,6 +1021,7 @@ virStorageVolDefPtr vol) { int fd; + char *kvmimg = NULL, *qemuimg = NULL, *qcowcreate = NULL;

if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW) { if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, @@ -1095,61 +1096,94 @@ vol->target.path); return -1; } - } else { -#if HAVE_QEMU_IMG + } else if (((kvmimg = virFindFileInPath("kvm-img")) != NULL) || + ((qemuimg = virFindFileInPath("qemu-img")) != NULL)) { const char *type = virStorageVolFormatFileSystemTypeToString(vol->target.format); const char *backingType = vol->backingStore.path ? virStorageVolFormatFileSystemTypeToString(vol->backingStore.format) : NULL; char size[100]; - const char **imgargv; - const char *imgargvnormal[] = { - QEMU_IMG, "create", "-f", type, vol->target.path, size, NULL, - }; - /* XXX including "backingType" here too, once QEMU accepts - * the patches to specify it. It'll probably be -F backingType */ - const char *imgargvbacking[] = { - QEMU_IMG, "create", "-f", type, "-b", vol->backingStore.path, vol->target.path, size, NULL, - }; + char *createtool; + const char *imgargv[11]; + int argvoffset; + + if (kvmimg != NULL) + imgargv[0] = kvmimg; + else if (qemuimg != NULL) + imgargv[0] = qemuimg; + else { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Can not find qemu-img or kvm-img")); + return -1; + } + + /* these values are always the same no matter the command */ + imgargv[1] = "create"; + imgargv[2] = "-f"; + imgargv[3] = type; + argvoffset = 4;

if (type == NULL) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, _("unknown storage vol type %d"), vol->target.format); + free(kvmimg); + free(qemuimg); return -1; } - if (vol->backingStore.path == NULL) { - imgargv = imgargvnormal; - } else { + + if (vol->backingStore.path != NULL) { if (backingType == NULL) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, _("unknown storage vol backing store type %d"), vol->backingStore.format); + free(kvmimg); + free(qemuimg); return -1; } + + /* XXX including "backingType" here too, once QEMU accepts + * the patches to specify it. It'll probably be -F backingType */ + if (kvmimg != NULL) { + imgargv[argvoffset++] = "-F"; + imgargv[argvoffset++] = backingType; + } + if (access(vol->backingStore.path, R_OK) != 0) { virReportSystemError(conn, errno, _("inaccessible backing store volume %s"), vol->backingStore.path); + free(kvmimg); + free(qemuimg); return -1; }

- imgargv = imgargvbacking; + imgargv[argvoffset++] = "-b"; + imgargv[argvoffset++] = vol->backingStore.path; }

+ imgargv[argvoffset++] = vol->target.path; + imgargv[argvoffset++] = size; + imgargv[argvoffset] = NULL; + /* Size in KB */ snprintf(size, sizeof(size), "%llu", vol->capacity/1024);

if (virRun(conn, imgargv, NULL) < 0) { + free(kvmimg); + free(qemuimg); return -1; }

+ free(kvmimg); + free(qemuimg); + if ((fd = open(vol->target.path, O_RDONLY)) < 0) { virReportSystemError(conn, errno, _("cannot read path '%s'"), vol->target.path); return -1; } -#elif HAVE_QCOW_CREATE + } else if ((qcowcreate = virFindFileInPath("qcow-create")) != NULL) { /* * Xen removed the fully-functional qemu-img, and replaced it * with a partially functional qcow-create. Go figure ??!? @@ -1161,39 +1195,44 @@ virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, _("unsupported storage vol type %d"), vol->target.format); + free(qcowcreate); return -1; } if (vol->backingStore.path != NULL) { virStorageReportError(conn, VIR_ERR_NO_SUPPORT, _("copy-on-write image not supported with " "qcow-create")); + free(qcowcreate); return -1; }

/* Size in MB - yes different units to qemu-img :-( */ snprintf(size, sizeof(size), "%llu", vol->capacity/1024/1024);

- imgargv[0] = QCOW_CREATE; + imgargv[0] = qcowcreate; imgargv[1] = size; imgargv[2] = vol->target.path; imgargv[3] = NULL;

if (virRun(conn, imgargv, NULL) < 0) { + free(qcowcreate); return -1; }

+ free(qcowcreate); + if ((fd = open(vol->target.path, O_RDONLY)) < 0) { virReportSystemError(conn, errno, _("cannot read path '%s'"), vol->target.path); return -1; } -#else + } else { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("creation of non-raw images " - "is not supported without qemu-img")); + "is not supported without kvm-img or " + "qemu-img or qcow-create")); return -1; -#endif }

In this piece of the code, rather than adding free(XXX) many times throughout the method, change all the 'return -1' calls into a 'goto error' and have a single piece of cleanup code at the end.

/* We can only chown/grp if root */ diff -Nur libvirt-0.6.3/src/util.c libvirt-0.6.3-kvm-img/src/util.c --- libvirt-0.6.3/src/util.c 2009-04-24 04:46:45.000000000 -0500 +++ libvirt-0.6.3-kvm-img/src/util.c 2009-05-27 16:54:23.000000000 -0500 @@ -1002,6 +1002,38 @@ return(0); }

+/* + * Find a file in the PATH. You must free + * the result + */ +char *virFindFileInPath(const char *find) +{ + char pathenv[PATH_MAX]; + char *pathseg; + char *fullpath; + + /* copy PATH env so we can tweak it */ + strncpy(pathenv, getenv("PATH"), PATH_MAX); + pathenv[PATH_MAX - 1] = '\0'; + + /* buffer for our file path */ + fullpath = malloc(PATH_MAX); + if (fullpath == NULL) + return NULL; + + /* for each path segment, append the file name to search for + * and test for it. return it if successful */ + while ((pathseg = strsep(&pathenv, ":")) != NULL) { + snprintf(fullpath, PATH_MAX, "%s/%s", pathseg, find); + if (virFileExists(fullpath)) + return fullpath; + } + + free(fullpath); + + return NULL; +} +

I don't much like this function with the mix of fixe length buffers, and fixed length malloc()'s. I think it'd be better to split out the code for breaking $PATH into a list of strings into a separate function, eg int virSplitPath(const char *src, const char **dst); So it'd take the $PATH value, and return a list of strings in 'dst', and the number of strins as the return value. Then the virFindFileInPath() method, would be better to iterate over this, and use virAsprintf() to build the full path, rather than relying on fixed size buffers. The s/free/VIR_FREE/, and s/malloc/VIR_ALLOC/ is also needed 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 :|