2:58 p.m.
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
4:12 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:04 a.m.
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 :|
10:58 p.m.
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
1:02 p.m.
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 :|
10:17 p.m.
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;
}
3:16 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
8:33 a.m.
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
11:29 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5732
days inactive
5760
days old
8 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard