Re: [libvirt] [PATCHv4 8/8] storage: probe qcow2 volumes in gluster pool

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).