Daniel P. Berrange wrote:
On Tue, Aug 12, 2008 at 11:58:07PM -0400, Cole Robinson wrote:
Daniel P. Berrange wrote:
This isn't correct because the target path is not guarenteed to point to the master device name /dev/sda1. The user could have configured it to use a stable path such as /dev/disk/by-uuid/4cb23887-0d02-4e4c-bc95-7599c85afc1a
Hmm, I couldn't actually get /dev/disk/by-uuid to work. Seems like the vol populating code for disks doesn't take into account the the pools target path, and just uses the real partition path.
Yes it does - this is what the virStorageBackendStablePath() method call does. What I expect is going on is that you merely created a bunch of partitions, but don't have any filesystems formatted in them. The UUID stuff is actually the UUID of the filesystem. If you try with a target path of /dev/disk/by-path you'll probably have more luck. If it can't find a stable path under the target you give, it automatically falls back to the generic /dev/sdXX path.
The following config should show it in action
<pool type='disk'> <name>mydisk</name> <source> <device path='/dev/sda'> </device> </source> <target> <path>/dev/disk/by-path</path> </target> </pool>
Daniel
Alright! I've managed to get this working. The attached patch has no problem deleting disk volumes if using by-path, by-uuid, or typical /dev. One hiccup I ran into is that by-uuid symlinks point to ../../sdfoo, rather than explictly /dev/sdfoo, hence the use of basename in this patch. Thanks, Cole diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index b3e6692..883c8b7 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -22,12 +22,16 @@ */ #include <config.h> +#include <string.h> #include "internal.h" #include "storage_backend_disk.h" #include "util.h" #include "memory.h" +#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__) +#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg) + enum { VIR_STORAGE_POOL_DISK_DOS = 0, VIR_STORAGE_POOL_DISK_DVH, @@ -419,13 +423,6 @@ virStorageBackendDiskBuildPool(virConnectPtr conn, return 0; } - -static int -virStorageBackendDiskDeleteVol(virConnectPtr conn, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - unsigned int flags); - static int virStorageBackendDiskCreateVol(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -489,14 +486,61 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, static int virStorageBackendDiskDeleteVol(virConnectPtr conn, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - virStorageVolDefPtr vol ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, unsigned int flags ATTRIBUTE_UNUSED) { - /* delete a partition */ - virStorageReportError(conn, VIR_ERR_NO_SUPPORT, - _("Disk pools are not yet supported")); - return -1; + char *part_num = NULL; + int n; + char devpath[PATH_MAX]; + char *devname, *srcname; + + if ((n = readlink(vol->target.path, devpath, sizeof(devpath))) < 0 && + errno != EINVAL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Couldn't read volume target path '%s'. %s"), + vol->target.path, strerror(errno)); + return -1; + } else if (n <= 0) { + strncpy(devpath, vol->target.path, PATH_MAX); + } else { + devpath[n] = '\0'; + } + + devname = basename(devpath); + srcname = basename(pool->def->source.devices[0].path); + DEBUG("devname=%s, srcname=%s", devname, srcname); + + if (!STRPREFIX(devname, srcname)) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Volume path '%s' did not start with parent " + "pool source device name."), devname); + return -1; + } + + part_num = devname + strlen(srcname); + + if (!part_num) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot parse partition number from target " + "'%s'"), devname); + return -1; + } + + /* eg parted /dev/sda rm 2 */ + const char *prog[] = { + PARTED, + pool->def->source.devices[0].path, + "rm", + "--script", + part_num, + NULL, + }; + + if (virRun(conn, prog, NULL) < 0) + return -1; + + return 0; }