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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list