Re: [Libvir] PATCH: 12/16: logical volume backend
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/