Re: [libvirt] [PATCHv3 3/4] storage: implement rudimentary glusterfs pool refresh
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
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)