[libvirt] [PATCHv3 3/4] storage: implement rudimentary glusterfs pool refresh

[Eric Blake]

On 11/12/2013 08:35 AM, Peter Krempa wrote:
> On 11/12/13 05:19, Eric Blake wrote:
>> Actually put gfapi to use, by allowing the creation of a gluster
>> pool.  Right now, all volumes are treated as raw; further patches
>> will allow peering into files to allow for qcow2 files and backing
>> chains, and reporting proper volume allocation.
>>
>> I've reported a couple of glusterfs bugs; if we were to require a
>> minimum of (not-yet-released) glusterfs 3.5, we could use the new
>> glfs_readdir [1] and not worry about the bogus return value of
>> glfs_fini [2], but for now I'm testing with Fedora 19's glusterfs
>> 3.4.1.
> 
> I'm not sure if this paragraph is suitable in a commit message.

Yes, because it justifies why we require a minimum version of gluster,
and what it would take to bump that minimum version (whether up or
down).  But perhaps it belongs better in 1/4 when we actually do
autoconf probing for a particular glusterfs version.  I'll move it.

>> +
>> +    /* Yuck - glusterfs-api-3.4.1 appears to always return -1 for
>> +     * glfs_fini, with errno containing random data, so there's no way
>> +     * to tell if it succeeded. 3.4.2 is supposed to fix this.*/
>> +    if (state->vol && glfs_fini(state->vol) < 0)
>> +        VIR_DEBUG("shutdown of gluster volume %s failed with errno %d",
>> +                  state->volname, errno);
> 
> I'm not sure if we can do anything else if close of the volume fails.
> The only change would be to report something more critical.

The problem is that gluster 3.4.1 _always_ reports failure, even when it
succeeds.  If we were to require a minimum of (not-yet-released) 3.4.2,
where that gluster bug is fixed, then yes we could log something more
critical on failure to close.

>> +static virStorageBackendGlusterStatePtr
>> +virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
>> +{
>> +    virStorageBackendGlusterStatePtr ret = NULL;
>> +    char *sub;
>> +    const char *name = pool->def->source.name;
>> +
>> +    if (VIR_ALLOC(ret) < 0)
>> +        return NULL;
>> +
>> +    if (*name == '/')
>> +        name++;
> 
> Wouldn't it be better just to forbid names with slashes? or trim them
> away when creating the pool? This would avoid multiple of such hacks.

Dan asked a similar question on 1/4; I guess we need to make sure we are
happy with the XML representation (or whether I need to revisit things
to make the XML specify volume separately from subdirectory, rather than
my current attempt to hack volume/subdir into a single name).

>> +    /* FIXME: Currently hard-coded to tcp transport; XML needs to be
>> +     * extended to allow alternate transport */
> 
> Okay for now IMHO.
> 
>> +    if (VIR_ALLOC(ret->uri) < 0 ||
>> +        VIR_STRDUP(ret->uri->scheme, "gluster") < 0 ||
>> +        VIR_STRDUP(ret->uri->server, pool->def->source.hosts[0].name) < 0 ||
>> +        virAsprintf(&ret->uri->path, "%s%s",
>> +                    *pool->def->source.name == '/' ? "" : "/",
>> +                    pool->def->source.name) < 0)
> 
> Same as above, if we would trim away the slash, we could add it without
> the need to check for it.

And maybe I should just canonicalize what the user passed in, so that
the rest of the code doesn't have to keep checking.  But one thing I did
do well in this patch was isolating the '/' games to just the open
method; the rest of the code reuses details learned here, rather than
repeating the conditionals.

>> +        virReportSystemError(errno, _("failed to connect to %s"),
>> +                             NULLSTR(uri));
> 
> I'd suggest adding apostrophes around %s to denote the URI as we do in
> other places.

Done.

>> +    /* Silently skip directories, including '.' and '..'.  FIXME:
>> +     * should non-'.' subdirectories be listed as type dir?  */
> 
> Is it even possible to use them with qemu as dir-type volumes?

Yes, a 'directory' pool lists dir-type volumes for all subdirectories,
skipping only '.' and '..'.  I'll see about wiring that in, and whether
it's easier to respin this patch or treat it as a followup.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131112/147fdc27/attachment-0001.sig>