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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/