On 11/25/2013 08:59 AM, Daniel P. Berrange wrote:
On Fri, Nov 22, 2013 at 08:20:30PM -0700, Eric Blake wrote:
Putting together pieces from previous patches, it is now possible for 'virsh vol-dumpxml --pool gluster volname' to report metadata about a qcow2 file stored on gluster. The backing file is still treated as raw; to fix that, more patches are needed to make the storage backing chain analysis recursive rather than halting at a network protocol name, but that work will not need any further calls into libgfapi so much as just reusing this code, and that should be the only code outside of the storage driver that needs any help from libgfapi. Any additional use of libgfapi within libvirt should only be needed for implementing storage pool APIs such as volume creation or resizing, where backing chain analysis should be unaffected.
+ while (maxlen) { + ssize_t r = glfs_read(fd, s, maxlen, 0); + if (r < 0 && errno == EINTR) + continue; + if (r < 0) { + VIR_FREE(*buf); + virReportSystemError(errno, _("unable to read '%s'"), name); + return r; + }
Further down you're requesting O_NONBLOCK, and here you are not handling EAGAIN explicitly. Is is desirable that we turn EAGAIN into a fatal error, or should we remove the O_NONBLOCK flag ?
Hmm. I was copying from directory pools, which also use O_NONBLOCK then call into virFileReadHeaderFD. So that code needs to be fixed (separate patch). For gluster, I think it's easiest to just drop O_NONBLOCK from the code (I just verified that attempts to use 'mkfifo' inside a gluster volume fail with EACCES); for directory pools you DO want to open O_NONBLOCK (otherwise opening a fifo for read would hang waiting for a writer), then use virSetNonBlock() after verifying file type but before reading the header (since we already reject fifos as unable to be used as a storage volume). Squashing in this, then pushing. diff --git i/src/storage/storage_backend_gluster.c w/src/storage/storage_backend_gluster.c index 71edb12..7e5ea9e 100644 --- i/src/storage/storage_backend_gluster.c +++ w/src/storage/storage_backend_gluster.c @@ -245,7 +245,9 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, vol->type = VIR_STORAGE_VOL_NETWORK; vol->target.format = VIR_STORAGE_FILE_RAW; - if (!(fd = glfs_open(state->vol, name, O_RDONLY| O_NONBLOCK | O_NOCTTY))) { + /* No need to worry about O_NONBLOCK - gluster doesn't allow creation + * of fifos, so there's nothing it would protect us from. */ + if (!(fd = glfs_open(state->vol, name, O_RDONLY | O_NOCTTY))) { /* A dangling symlink now implies a TOCTTOU race; report it. */ virReportSystemError(errno, _("cannot open volume '%s'"), name); goto cleanup; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org