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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list