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 -=|