Re: [libvirt] [PATCH] Implement vol delete for disk pools

13 Aug 2008


      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