Re: [libvirt] [PATCH 2/4] Avoid create/unlink with block devs used for QEMU save

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.