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

[Daniel P. Berrange]

On Tue, Feb 19, 2008 at 03:43:11AM -0500, Daniel Veillard wrote:
> > 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

Yes, I found this because the LVM tools will print out a warning
message if they're passed any file descriptor > 2 !

> Seems some of the code tries to be generic, what other kind of
> logical volume do you have in mind ?

Solaris has ZFS which provides parity with LVM volume manegement
so I intend that they'd have a ZFS impl of 'logical' pool type

> [...]
> > +
> > +    /* 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.

The caller will free the entire object & shutdown the pool if this fails.
I should however, report the error message before returning.

> [...]
> > +    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

Yep.

> > +
> > +        /*
> > +         * 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 ...

The 'pvcreate' man page explicitly says the first sector

<quote>
   For whole disk devices only the partition table must be erased, 
   which will effectively destroy all data on that disk.  This can 
   be done by zeroing the  first  sector with:

       dd if=/dev/zero of=PhysicalVolume bs=512 count=1
</quote>

So 512 is fine for MSDOS partition tables at least.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|