[libvirt] [PATCH] Implement vol delete for disk pools
The patch below implements virStorageVolDelete for volumes on a disk pool. The only interesting thing here is that parted wants a partition number to delete, so we need to peel off the end of the volume's target path which will be of the form '/dev/sda1' or similar (I assume. If not, it's still better than having nothing implemented). Thanks, Cole
On Mon, Aug 11, 2008 at 03:58:41PM -0400, Cole Robinson wrote:
The patch below implements virStorageVolDelete for volumes on a disk pool.
The only interesting thing here is that parted wants a partition number to delete, so we need to peel off the end of the volume's target path which will be of the form '/dev/sda1' or similar (I assume. If not, it's still better than having nothing implemented).
Without much understanding of the storage command, I will say the patch looks fine to me, but I would prefer another review :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Aug 11, 2008 at 03:58:41PM -0400, Cole Robinson wrote:
The patch below implements virStorageVolDelete for volumes on a disk pool.
The only interesting thing here is that parted wants a partition number to delete, so we need to peel off the end of the volume's target path which will be of the form '/dev/sda1' or similar (I assume. If not, it's still better than having nothing implemented).
Thanks, Cole
diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index dac827b..28e0a52 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -22,11 +22,13 @@ */
#include <config.h> +#include <string.h>
#include "internal.h" #include "storage_backend_disk.h" #include "util.h" #include "memory.h" +#include "c-ctype.h"
enum { VIR_STORAGE_POOL_DISK_DOS = 0, @@ -416,13 +418,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, @@ -486,14 +481,41 @@ 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 i; + + /* Strip target path (ex. '/dev/sda1') of its partition number */ + for (i = (strlen(vol->target.path)-1); i >= 0; --i) { + if (!c_isdigit(vol->target.path[i])) + break; + part_num = &(vol->target.path[i]); + } + + if (!part_num) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot parse partition number from target " + "path '%s'"), vol->target.path); + return -1; + }
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 So, you need to first call 'readlink' on the vol->target.path, ignoring EINVAL which you'll get if it wasn't a symlink. Once you've done that you'll need to validate that it is sane by checking that vol->source.devices[0] is a prefix of the resolved pathname. Finally, you can extract the partition number. So something along the lines of char devname[PATH_MAX]; if (readlink(vol->target.path, devname, sizeof(devname)) < 0 && errno != EINVAL) virStorageReportError(...) if (!STRPREFIX(devname, vol->source.devices[0].path)) virStorageReportError(....) part_num = devname + strlen(vol->source.devices[0].path)
+ + /* eg parted /dev/sda rm 2 */ + const char *prog[] = { + PARTED, + pool->def->source.devices[0].path, + "rm", + "--script", + part_num, + NULL, + }; +
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 :|
Daniel P. Berrange wrote:
On Mon, Aug 11, 2008 at 03:58:41PM -0400, Cole Robinson wrote:
The patch below implements virStorageVolDelete for volumes on a disk pool.
The only interesting thing here is that parted wants a partition number to delete, so we need to peel off the end of the volume's target path which will be of the form '/dev/sda1' or similar (I assume. If not, it's still better than having nothing implemented).
Thanks, Cole
{ - /* delete a partition */ - virStorageReportError(conn, VIR_ERR_NO_SUPPORT, - _("Disk pools are not yet supported")); - return -1; + char *part_num = NULL; + int i; + + /* Strip target path (ex. '/dev/sda1') of its partition number */ + for (i = (strlen(vol->target.path)-1); i >= 0; --i) { + if (!c_isdigit(vol->target.path[i])) + break; + part_num = &(vol->target.path[i]); + } + + if (!part_num) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot parse partition number from target " + "path '%s'"), vol->target.path); + return -1; + }
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.
So, you need to first call 'readlink' on the vol->target.path, ignoring EINVAL which you'll get if it wasn't a symlink. Once you've done that you'll need to validate that it is sane by checking that vol->source.devices[0] is a prefix of the resolved pathname. Finally, you can extract the partition number. So something along the lines of
char devname[PATH_MAX];
if (readlink(vol->target.path, devname, sizeof(devname)) < 0 && errno != EINVAL) virStorageReportError(...)
if (!STRPREFIX(devname, vol->source.devices[0].path)) virStorageReportError(....)
part_num = devname + strlen(vol->source.devices[0].path)
The attached patch uses this approach. It works for the case with vols of the form /dev/sdx123, but as mentioned above I couldn't get by-uuid method to work, so can't be certain about that. Thanks, Cole
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 -- |: 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 :|
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; }
On Fri, Sep 05, 2008 at 11:17:27PM -0400, Cole Robinson wrote:
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);
This seems to leak the two strings and not check for errors. That would need to be fixed before being commited IMHO
+ 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; }
Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Fri, Sep 05, 2008 at 11:17:27PM -0400, Cole Robinson wrote:
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);
This seems to leak the two strings and not check for errors. That would need to be fixed before being commited IMHO
glibc defines basename in string.h as: char * basename (filename) const char *filename; { char *p = strrchr (filename, '/'); return p ? p + 1 : (char *) filename; } So there doesn't seem to be any cleanup or failure catching necessary. Thanks, Cole
On Mon, Sep 08, 2008 at 09:33:40AM -0400, Cole Robinson wrote:
Daniel Veillard wrote:
+ devname = basename(devpath); + srcname = basename(pool->def->source.devices[0].path);
This seems to leak the two strings and not check for errors. That would need to be fixed before being commited IMHO
glibc defines basename in string.h as:
char * basename (filename) const char *filename;
I got fooled by the char * return value, my bad, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard