Re: [libvirt] [PATCH] Implement vol delete for disk pools
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/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Implement vol delete for disk pools
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 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Implement vol delete for disk pools
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/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Implement vol delete for disk pools
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 > > > mydisk > > > > > > /dev/disk/by-path > > > > 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 +#include #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; } -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Implement vol delete for disk pools
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 mydisk /dev/disk/by-path 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Implement vol delete for disk pools
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 diff --git a/src/storage_backend_disk.c b/src/storage_backend_disk.c index 31a35b5..889ed66 100644 --- a/src/storage_backend_disk.c +++ b/src/storage_backend_disk.c @@ -22,6 +22,7 @@ */ #include +#include #include "internal.h" #include "storage_backend_disk.h" @@ -417,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, @@ -487,14 +481,56 @@ 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, n; +char devname[PATH_MAX]; + +if ((n = readlink(vol->target.path, devname, sizeof(devname))) < 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(devname, vol->target.path, PATH_MAX); +} else { +devname[n] = '\0'; +} + +if (!STRPREFIX(devname, pool->def->source.devices[0].path)) { +virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Volume path did not start with parent pool " +"path.")); +return -1; +} + +part_num = devname + strlen(pool->def->source.devices[0].path); + +if (!part_num) { +virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _(
Re: [libvirt] [PATCH] Implement vol delete for disk pools
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 > +#include > > #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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Implement vol delete for disk pools
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/ [EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[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 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 +#include #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; +} + +/* 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; } -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list