On Wed, Jul 30, 2008 at 04:30:01PM -0400, David Lively wrote:

For example, to discover existing LVM volume groups, there's no need to specify a source: virsh # pool-discover logical

While discovering nfs shares requires a <source> document to specify the host to query: virsh # pool-discover netfs "<source><host name='share' /></source>"

Currently (i.e., in this patch) I've implemented discovery support only for the "logical" and "netfs" pools. I plan on covering the other types once we've agreed upon the API. While this patch is rather large, the vast majority of it is "generic" (not specific to any particular type of storage pool) code, which I mostly took from Daniel's submission. I could easily break the storage-pool-type-specific pieces into separate patches, but those changes are already segregated into the appropriate storage_backend_ files.

* Known Issues *

I made the virsh interface take a XML string rather than a filename. I found it convenient, but AFAIK the rest of the commands that take XML take it from a file, so perhaps that should be changed (just let me know what you'd prefer).

I'd rather this took at filename, and then we add a separate pool-discover-as command which takes a list of flags and turns them into XML. eg, so you could do virsh pool-discover-as --host share netfs and thus avoid the user needing to deal with XML at all here.

I realize the description of srcSpec is kind of fuzzy. For instance, for netfs discovery, the <source> must have a <host> element (to specify the host to query), but shouldn't have a <dir> element since the exported dirs are exactly what we're trying to discover in this case. So this really needs to be documented accurately for each storage pool type. (Where?)

We've got some documentation on the XML required for each type of storage pool here, so its natural to add more info in this page http://libvirt.org/storage.html It could also be detailed in the virsh manpage for 'pool-discover' command.

Finally, there's an underspecification issue. The XML pool descriptions returned are supposed to be usable as valid input to virStoragePoolDefineXML. But these descriptions include some data (pool name, target path) that isn't specified by the discover input or the discovered resources. For now, I'm making up a somewhat arbitrary pool name (logical: VolGroup name, netfs: last component of export path). And I don't even specify <target><path> in the "netfs" discovery output (which means it's not valid input to virStoragePoolDefineXML until a target path is added).

Yep, my original intention was that you could send the XML straight back into the PoolDefineXML api. One alternative I've thought of though, is that it perhaps it might be nicer to return all the discovered pools as one XML doc <poolList type='netfs'> <source> <host name='someserver'/> <dir path='/exports/home'/> </source> <source> <host name='someserver'/> <dir path='/exports/web'/> </source> <source> <host name='someserver'/> <dir path='/exports/ftp'/> </source> </poolList> And just let the caller decide how they want to use this - they may not even want to define pools from them immediately - instead they might just want to display list of NFS exports to the user. It'd be easier than having to make up data for the <name> and <target> elements which is really something the client app wants to decide upon.

diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h index 8980bc2..463cf2b 100644 --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -890,6 +890,15 @@ int virConnectListDefinedStoragePools(virConnectPtr conn, int maxnames);

/* + * Query a host for storage pools of a particular type + */ +int virConnectDiscoverStoragePools(virConnectPtr conn, + const char *type, + const char *srcSpec, + unsigned int flags, + char ***xmlDesc); + +/* * Lookup pool by name or uuid */ virStoragePoolPtr virStoragePoolLookupByName (virConnectPtr conn, @@ -979,6 +988,13 @@ char * virStorageVolGetXMLDesc (virStorageVolPtr pool,

char * virStorageVolGetPath (virStorageVolPtr vol);

+typedef struct _virStringList virStringList; + +struct _virStringList { + char *val; + struct _virStringList *next; +};

This typedef doesn't appear to be needed in the public API, so can be moved internally.

diff --git a/python/generator.py b/python/generator.py index 1514c02..66cb3cc 100755 --- a/python/generator.py +++ b/python/generator.py @@ -313,6 +313,7 @@ skip_impl = ( 'virStorageVolGetInfo', 'virStoragePoolGetAutostart', 'virStoragePoolListVolumes', + 'virConnectDiscoverStoragePools' )

Ok, guess we'll need to code this up in python manually. Although if we changed to only returning one XML doc with all discovered poool the API contract would be simpler char *virStoragePoolDiscover(virConnectPtr conn, char *type, char *srcSpec) and that would be able to work with the auto-generator no trouble.

diff --git a/qemud/remote.c b/qemud/remote.c index b5a6ec9..f2814e9 100644 --- a/qemud/remote.c +++ b/qemud/remote.c @@ -2958,6 +2958,41 @@ remoteDispatchListStoragePools (struct qemud_server *server ATTRIBUTE_UNUSED, }

static int +remoteDispatchDiscoverStoragePools (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client, + remote_message_header *req, + remote_discover_storage_pools_args *args, + remote_discover_storage_pools_ret *ret) +{ + CHECK_CONN(client); + char **xmlDesc = NULL; + + /* Allocate return buffer. */ + ret->xml.xml_len = + virConnectDiscoverStoragePools (client->conn, + args->type, + args->srcSpec ? *args->srcSpec : NULL, + args->flags, + &xmlDesc); + if (ret->xml.xml_len == -1) return -1; + + if (ret->xml.xml_len > REMOTE_STORAGE_POOL_NAME_LIST_MAX) { + int i; + for (i = 0 ; i < ret->xml.xml_len ; i++) + free(xmlDesc[i]); + free(xmlDesc);

Recently we've switched to using VIR_FREE instead - see memory.h or HACKING for not details.

diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index f195034..75013e8 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -35,12 +35,18 @@ #include <byteswap.h> #include <mntent.h> #include <string.h> +#include "c-ctype.h" + +#include <libxml/parser.h> +#include <libxml/tree.h> +#include <libxml/xpath.h>

#include "internal.h" #include "storage_backend_fs.h" #include "storage_conf.h" #include "util.h" #include "memory.h" +#include "xml.h"

enum { VIR_STORAGE_POOL_FS_AUTO = 0, @@ -442,6 +448,149 @@ static int virStorageBackendProbeFile(virConnectPtr conn, }

#if WITH_STORAGE_FS +struct _virNetfsDiscoverState { + const char *host; + virStringList *list; +}; + +typedef struct _virNetfsDiscoverState virNetfsDiscoverState; + +static int +virStorageBackendFileSystemNetDiscoverPoolsFunc(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + char **const groups, + void *data) +{ + virNetfsDiscoverState *state = data; + virStringList *newItem; + const char *name, *path; + int len; + + path = groups[0]; + + name = rindex(path, '/'); + if (name == NULL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("no / in path?")); + return -1; + }

There's some emacs rules in the HACKING file which will make sure it does correct indentation for you. BTW, anyone fancy adding VIM equivalents...

+ name += 1; + + /* Append new XML desc to list */ + + if (VIR_ALLOC(newItem) != 0) { + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("new xml desc")); + return -1; + } + + + /* regexp pattern is too greedy, so we may have trailing spaces to trim.

Does the regex engine allow .*? to specify non-greedy matches ?

+static int +virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn, + const char *srcSpec, + unsigned int flags ATTRIBUTE_UNUSED, + char ***xmlDescs) +{ + /* + * # showmount --no-headers -e HOSTNAME + * /tmp * + * /A dir demo1.foo.bar,demo2.foo.bar + * + * Extract directory name (including possible interior spaces ...). + */ + + const char *regexes[] = { + "^(/.*) +\\S+$" + }; + int vars[] = { + 1 + }; + xmlDocPtr doc = NULL; + xmlXPathContextPtr xpath_ctxt = NULL; + virNetfsDiscoverState state = { .host = NULL, .list = NULL }; + const char *prog[] = { SHOWMOUNT, "--no-headers", "--exports", NULL, NULL }; + int exitstatus, n_descs = -1; + virStringList *p; + + doc = xmlReadDoc((const xmlChar *)srcSpec, "srcSpec.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOERROR | XML_PARSE_NOWARNING); + if (doc == NULL) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, "%s", _("bad <source> spec")); + goto cleanup;

Whitespace issue - and others following - i won't list them all In general the patch looks good to me. The impl for LVM and NFS were nice & straightforward - easier than I expected in fact. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Hi David, I spotted a few things that would be good to change: David Lively <dlively@virtualiron.com> wrote:

diff --git a/configure.in b/configure.in index 8e04f14..5ef0384 100644 --- a/configure.in +++ b/configure.in @@ -660,6 +660,10 @@ if test "$with_storage_fs" = "yes" -o "$with_storage_fs" = "check"; then fi fi AM_CONDITIONAL([WITH_STORAGE_FS], [test "$with_storage_fs" = "yes"]) +if test "$with_storage_fs" = "yes"; then + AC_PATH_PROG([SHOWMOUNT], [showmount], [], [$PATH:/sbin:/usr/sbin]) + AC_DEFINE_UNQUOTED([SHOWMOUNT], ["$SHOWMOUNT"], [Location or name of the showmount program])

Please split long lines: AC_DEFINE_UNQUOTED([SHOWMOUNT], ["$SHOWMOUNT"], [Location or name of the showmount program]) ...

diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c ... +static int +virStorageBackendFileSystemNetDiscoverPoolsFunc(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + char **const groups, + void *data) +{ + virNetfsDiscoverState *state = data; + virStringList *newItem; + const char *name, *path; + int len; + + path = groups[0]; + + name = rindex(path, '/');

rindex is deprecated. Using it causes portability problems. Use strrchr instead.

+ if (name == NULL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("no / in path?"));

If you include the offending string in the diagnostic and add a word of description, it'll be more useful: virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, _("invalid netfs path (no slash): %s"), path);

+ return -1; + } + name += 1; + + /* Append new XML desc to list */ + + if (VIR_ALLOC(newItem) != 0) { + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("new xml desc")); + return -1; + } + + + /* regexp pattern is too greedy, so we may have trailing spaces to trim. + * NB showmount output ambiguous for (evil) exports with names w/trailing whitespace

Too long.

+ */ + len = 0; + while (name[len]) + len++;

This is equivalent to the three lines above: len = strlen (name);

+ while (c_isspace(name[len-1])) + len--;

It is customary to trim spaces and TABs (but less so CR, FF, NL), so c_isblank might be better than c_isspace here. ...

+static int +virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn, ... + cleanup: + if (doc) + xmlFreeDoc(doc); + if (xpath_ctxt)

You can remove this "if" test. It's unnecessary. BTW, "make syntax-check" should spot this and fail because of it.

+ xmlXPathFreeContext(xpath_ctxt); + VIR_FREE(state.host); + while (state.list) { + p = state.list->next; + VIR_FREE(state.list); + state.list = p; + } + + return n_descs; +} ... diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c index 9a0c27f..3c16d20 100644 --- a/src/storage_backend_logical.c +++ b/src/storage_backend_logical.c ... @@ -257,6 +258,90 @@ virStorageBackendLogicalRefreshPoolFunc(virConnectPtr conn ATTRIBUTE_UNUSED,

static int +virStorageBackendLogicalDiscoverPoolsFunc(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + size_t n_tokens, + char **const tokens, + void *data) +{ + virStringList **rest = data; + virStringList *newItem; + const char *name; + int len; + + /* Append new XML desc to list */ + + if (VIR_ALLOC(newItem) != 0) { + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("new xml desc")); + return -1; + } + + if (n_tokens != 1) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("n_tokens should be 1")); + return -1; + } + + + name = tokens[0]; + virSkipSpaces(&name);

Is is necessary to skip leading backslashes and carriage returns here?

+ len = 0; + while (name[len]) + len++; + while (c_isspace(name[len-1])) + len--;

A zero-length or all-"space" name would lead to referencing name[-1]. You can use this instead: while (len && c_isspace(name[len-1])) len--; The similar code above isn't vulnerable like this due to its guarantee that there is a "/".