On Thu, Apr 22, 2010 at 11:35:17AM +0100, Daniel P. Berrange wrote:
On Wed, Apr 21, 2010 at 02:49:42PM -0600, Eric Blake wrote:
On 04/21/2010 10:56 AM, Daniel P. Berrange wrote:
It is possible to use block devices with domain save/restore. Upon failure QEMU unlinks the path being saved to. This isn't good when it is a block device !
+ struct stat sb; + int is_bdev = 0;
Should this be bool instead of int?
+ /* path might be a pre-existing block dev, in which case + * we need to skip the create step, and also avoid unlink + * in the failure case */ + if (stat(path, &sb) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, _("unable to access %s"), path); + goto endjob; + } else { + is_bdev = 0; + } + } else { + is_bdev = S_ISBLK(sb.st_mode);
although if you use bool, you'd have to explicitly compare against 0 here, based on the gnulib restraints on stdbool.h. Also, what about other non-regular files? A directory will fail on the open, but what about named fifos (on the other hand, is anyone crazy enough to try a non-seekable file as the file to open?). So I'm wondering if it's better to check for S_ISREG, changing is_bdev to is_reg, and changing the cleanup logic to only attempt unlink() if is_reg, rather than skipping if is_bdev.
+ } + + /* Setup hook data needed by virFileOperation hook function */ hdata.dom = dom; hdata.path = path; @@ -4849,7 +4866,8 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, /* Write header to file, followed by XML */
/* First try creating the file as root */ - if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY, + if (!is_bdev && + (rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
This is racy. You stat() prior to open()ing. Wouldn't it be better to open(), then fstat()?
That would require significant changes to the virFileOperation code which is not something I think is worth the effort here.
Actually, the whole thing is racy regardless of what we do, since QEMU itself will spawn another program which will re-open this file. To avoid a race, we'd need to switch to QEMU's 'fd:' based migration and pass the pre-opened file handle to it Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|