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. > > [1] http://lists.gnu.org/archive/html/gluster-devel/2013-10/msg00085.html > [2] http://lists.gnu.org/archive/html/gluster-devel/2013-10/msg00086.html > > * src/storage/storage_backend_gluster.c > (virStorageBackendGlusterRefreshPool): Initial implementation. > (virStorageBackendGlusterOpen, virStorageBackendGlusterClose): New > helper functions. > > Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> > --- > src/storage/storage_backend_gluster.c | 220 +++++++++++++++++++++++++++++++++- > 1 file changed, 215 insertions(+), 5 deletions(-) > > diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c > index 2863c73..bc90de9 100644 > --- a/src/storage/storage_backend_gluster.c > +++ b/src/storage/storage_backend_gluster.c > @@ -23,20 +23,230 @@ > > #include <glusterfs/api/glfs.h> > > -#include "virerror.h" > #include "storage_backend_gluster.h" > #include "storage_conf.h" > +#include "viralloc.h" > +#include "virerror.h" > +#include "virlog.h" > +#include "virstoragefile.h" > +#include "virstring.h" > +#include "viruri.h" > > #define VIR_FROM_THIS VIR_FROM_STORAGE > > +struct _virStorageBackendGlusterState { > + glfs_t *vol; > + > + /* Accept the same URIs as qemu's block/gluster.c: > + * gluster[+transport]://[server[:port]]/vol/[dir/]image[?socket=...] */ > + virURI *uri; > + > + char *volname; /* vol from URI */ > + char *dir; /* dir from URI, or "." */ > +}; > + > +typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState; > +typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr; > + > +static void > +virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) > +{ > + if (!state) > + return; > + > + /* 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. > + > + virURIFree(state->uri); > + VIR_FREE(state->volname); > + VIR_FREE(state->dir); > + VIR_FREE(state); > +} > + > +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. > + if ((sub = strchr(name, '/'))) { > + if (VIR_STRNDUP(ret->volname, name, sub - name) < 0 || > + VIR_STRDUP(ret->dir, sub) < 0) > + goto error; > + } else { > + if (VIR_STRDUP(ret->volname, pool->def->source.name) < 0 || > + VIR_STRDUP(ret->dir, ".") < 0) > + goto error; > + } > + > + /* 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. > + goto error; > + ret->uri->port = pool->def->source.hosts[0].port; > + > + if (!(ret->vol = glfs_new(ret->volname))) { > + virReportOOMError(); > + goto error; > + } > + > + if (glfs_set_volfile_server(ret->vol, "tcp", > + ret->uri->server, ret->uri->port) < 0 || > + glfs_init(ret->vol) < 0) { > + char *uri = virURIFormat(ret->uri); > + > + 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. > + VIR_FREE(uri); > + goto error; > + } > + > + return ret; > + > +error: > + virStorageBackendGlusterClose(ret); > + return NULL; > +} > + > + > +/* Populate *volptr for the given name and stat information, or leave > + * it NULL if the entry should be skipped (such as "."). Return 0 on > + * success, -1 on failure. */ > +static int > +virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, > + const char *pool, > + const char *name, > + struct stat *st, > + virStorageVolDefPtr *volptr) > +{ > + char *tmp; > + int ret = -1; > + virStorageVolDefPtr vol = NULL; > + > + /* 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? > + *volptr = NULL; > + if (S_ISDIR(st->st_mode)) > + return 0; > + > + if (VIR_ALLOC(vol) < 0 || > + VIR_STRDUP(vol->name, name) < 0 || > + virAsprintf(&vol->key, "%s%s/%s", > + *pool == '/' ? "" : "/", pool, vol->name) < 0) Yet another place that could be cleaned up with explicitly forbidding/stripping slash. > + goto cleanup; > + > + /* 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; > + tmp = state->uri->path; > + state->uri->path = vol->key; > + if (!(vol->target.path = virURIFormat(state->uri))) { > + state->uri->path = tmp; > + goto cleanup; > + } > + state->uri->path = tmp; > + > + *volptr = vol; > + vol = NULL; > + ret = 0; > +cleanup: > + virStorageVolDefFree(vol); > + return ret; > +} > > static int > virStorageBackendGlusterRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, > - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED) > + virStoragePoolObjPtr pool) > { > - virReportError(VIR_ERR_NO_SUPPORT, "%s", > - _("gluster pool type not fully supported yet")); > - return -1; > + int ret = -1; > + virStorageBackendGlusterStatePtr state = NULL; > + struct { > + struct dirent ent; > + /* See comment below about readdir_r needing padding */ > + char padding[MAX(1, 256 - (int) (sizeof(struct dirent) > + - offsetof(struct dirent, d_name)))]; > + } de; > + struct dirent *ent; > + glfs_fd_t *dir = NULL; > + struct stat st; > + struct statvfs sb; > + > + if (!(state = virStorageBackendGlusterOpen(pool))) > + goto cleanup; > + > + /* Why oh why did glfs 3.4 decide to expose only readdir_r rather > + * than readdir? POSIX admits that readdir_r is inherently a > + * flawed design, because systems are not required to define > + * NAME_MAX: http://austingroupbugs.net/view.php?id=696 > + * http://womble.decadent.org.uk/readdir_r-advisory.html > + * > + * Fortunately, gluster appears to limit its underlying bricks to > + * only use file systems such as XFS that have a NAME_MAX of 255; Basically any usable filesystem to time has NAME_MAX 255. There are a few exceptions like Reiser4 but those are not production ready yet. Also it's questionable if gluster would work correctly on such filesystem. > + * so we are currently guaranteed that if we provide 256 bytes of > + * tail padding, then we should have enough space to avoid buffer > + * overflow no matter whether the OS used d_name[], d_name[1], or > + * d_name[256] in its 'struct dirent'. > + * http://lists.gnu.org/archive/html/gluster-devel/2013-10/msg00083.html > + */ > + > + if (!(dir = glfs_opendir(state->vol, state->dir))) { > + virReportSystemError(errno, _("cannot open path '%s'"), > + pool->def->name); > + goto cleanup; > + } > + while (!(errno = glfs_readdirplus_r(dir, &st, &de.ent, &ent)) && ent) { > + virStorageVolDefPtr vol; > + int okay = virStorageBackendGlusterRefreshVol(state, > + pool->def->source.name, > + ent->d_name, &st, > + &vol); > + if (okay < 0) > + goto cleanup; > + if (vol && VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, > + vol) < 0) > + goto cleanup; > + } > + if (errno) { > + virReportSystemError(errno, _("failed to read directory '%s'"), > + pool->def->source.name); > + goto cleanup; > + } > + > + if (glfs_statvfs(state->vol, state->dir, &sb) < 0) { > + virReportSystemError(errno, _("cannot statvfs path '%s'"), pool->def->source.name); Indentation is off. > + goto cleanup; > + } > + > + pool->def->capacity = ((unsigned long long)sb.f_frsize * > + (unsigned long long)sb.f_blocks); > + pool->def->available = ((unsigned long long)sb.f_bfree * > + (unsigned long long)sb.f_frsize); > + pool->def->allocation = pool->def->capacity - pool->def->available; > + > + ret = 0; > +cleanup: > + if (dir) > + glfs_closedir(dir); > + virStorageBackendGlusterClose(state); > + if (ret < 0) > + virStoragePoolObjClearVols(pool); > + return ret; > } > > virStorageBackend virStorageBackendGluster = { > ACK with some nits fixed and questions answered. Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list