On 11/12/2013 09:58 AM, Daniel P. Berrange wrote:
> On Mon, Nov 11, 2013 at 09:19:30PM -0700, 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.
>>
>> + /* FIXME: Currently hard-coded to tcp transport; XML needs to be
>> + * extended to allow alternate transport */
>> + 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)
>> + goto error;
>
> I'm not a huge fan of chaining together statements like this,
> since some gdb versions are pretty awful at telling you which
> one of these 4 statements has the problem on segv.
Easy enough to split; so consider it done.
>> + /* Silently skip directories, including '.' and '..'. FIXME:
>> + * should non-'.' subdirectories be listed as type dir? */
>
> What behaviour does the 'fs' pool have in for non-'.' subdirs ?
> Seems we perhaps ought to match that.
Peter asked the same question - guess I'm stuck doing that :)
>> +
>> + /* FIXME - must open files to determine if they are non-raw */
>> + vol->type = VIR_STORAGE_VOL_NETWORK;
>> + vol->target.format = VIR_STORAGE_FILE_RAW;
>> + vol->capacity = vol->allocation = st->st_size;
>
> So gluster has no concept of sparse volumes ?
Gluster has sparse support; I need to flesh this out a bit more, based
on how directory pools do it (both pools have a struct stat to work
with); I also need to populate ownership details.
>> +
>> + if (!(state = virStorageBackendGlusterOpen(pool)))
>> + goto cleanup;
>
> What kind of overhead is there in opening / closing gluster connections ?
> Is it something we should just do in the pool start/stop methods, instead
> of on every operation like refresh.
I was just copying after rbd. In gdb, I see several threads spawned for
every refresh operation; but I can certainly try to rework things to
open at pool start instead. Should I do it on this patch, or save it
for a followup?
The other consideration is that eventually, the
src/util/virstoragefile.c code will need to call into the
src/storage/storage_driver.c code (indirectly, via the virConnectPtr)
when doing recursive backing chain checking; in that case, I'm
envisioning a new callback (internal-only, no public API needed) that
roughly says: "given this URI for a backing file, tell me if you have a
backend for parsing the file, and if so, grab the header from that
file"; which means my ultimate goal is to have a reusable method that
both the pool XML (given host and name, [or host, volume, subdir, based
on review of 1/4]) and the qemu URI (given a string
"gluster://host/vol/..."). So while pool start may be better than pool
refresh for opening a glfs connection, the ability to open a glfs
connection also needs to be modular enough to reuse for one-off parsing
of a URI.
>> + if (glfs_statvfs(state->vol, state->dir, &sb) < 0) {
>> + virReportSystemError(errno, _("cannot statvfs path '%s'"), pool->def->source.name);
>
> Seems you lost the line break here.
Yep, Peter caught it too.
--
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