[Libvir] PATCH: 12/16: logical volume backend

[Daniel Veillard]

On Tue, Feb 12, 2008 at 04:38:39AM +0000, Daniel P. Berrange wrote:
> This patch implements a storage pool based on logical volume
> management. The underlying implementation calls out to LVM
> on Linux. A future implementation for Solaris would use the
> ZFS pool support to achieve the same functionality. The LVM
> impl uses the LVM command line tools, in particular lvs, and
> vgs for listing, and the *create, *remove tools for modifications.
> The 'build' operation will construct a new volume group, and
> initialize any physical volume members. The 'delete' operation
> will permanently remove the group. Volumes are allocated by
> carving out logical volumes. There are no placement constraints
> how the volume XML will show the actual storage per-volume on
> the underlying physical volumes. The LVM UUID is used for the
> unique volume keys.

  Okay, it would be good to get it tested on other Linux distros

> diff -r 31abfd8687b3 qemud/qemud.c
> --- a/qemud/qemud.c	Thu Feb 07 13:44:25 2008 -0500
> +++ b/qemud/qemud.c	Thu Feb 07 14:17:16 2008 -0500
> @@ -2089,7 +2089,9 @@ int main(int argc, char **argv) {
>  
>      if (pipe(sigpipe) < 0 ||
>          qemudSetNonBlock(sigpipe[0]) < 0 ||
> -        qemudSetNonBlock(sigpipe[1]) < 0) {
> +        qemudSetNonBlock(sigpipe[1]) < 0 ||
> +        qemudSetCloseExec(sigpipe[0]) < 0 ||
> +        qemudSetCloseExec(sigpipe[1]) < 0) {
>          qemudLog(QEMUD_ERR, _("Failed to create pipe: %s"),
>                   strerror(errno));
>          goto error1;

  That can be commited completely independantly, its a bug fix
Seems some of the code tries to be generic, what other kind of
logical volume do you have in mind ?

[...]
> +
> +    /* Finally fill in extents information */
> +    if ((tmp = realloc(vol->source.extents, sizeof(*tmp) * (vol->source.nextent + 1))) == NULL) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, "extents");
> +        return -1;
> +    }
> +    vol->source.extents = tmp;
> +
> +    if ((vol->source.extents[vol->source.nextent].path =
> +         strdup(groups[2])) == NULL) {
> +        virStorageReportError(conn, VIR_ERR_NO_MEMORY, "extents");
> +        return -1;
> +    }
> +
> +    if (xstrtol_ull(groups[3], NULL, 10, &offset) < 0)
> +        return -1;
> +    if (xstrtol_ull(groups[4], NULL, 10, &length) < 0)
> +        return -1;
> +    if (xstrtol_ull(groups[5], NULL, 10, &size) < 0)
> +        return -1;

  Can we really just return -1 here for error handling at that point ?
vol->source had had some of its fields filled, but incompletely initialized
this looks dangerous.

[...]
> +    for (i = 0 ; i < pool->def->source.ndevice ; i++) {
> +        int fd;
> +        char zeros[512];
> +        memset(zeros, 0, sizeof(zeros));

  those 2 can probably be moved out of the loop

> +
> +        /*
> +         * LVM requires that the first 512 bytes are blanked if using
> +         * a whole disk as a PV. So we just blank them out regardless
> +         * rather than trying to figure out if we're a disk or partition
> +         */

  is it really 512 or the block size on the device used ? But 512 is
probably sufficient for LVM to consider it cleared, just wondering ...

  Looks fine to me, +1

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/