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