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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list