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()?
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list