Re: [libvirt] [PATCH] storage pool discovery
On Thu, Aug 21, 2008 at 10:29:43AM -0400, David Lively wrote:
Hi Folks -
Here's my second pass at storage pool discovery. I've taken Daniel's
suggestion and made it return a single XML doc containing source
elements rather than an array of pool docs (and also incorporated
suggestions from Daniel V and Jim M).
Note that the storage source name patch is closely related
(without it, the source docs returned for logical pools aren't
correct).
Excellant, I think this patch looks more or less good to commit now.
+static char *
+virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
+const char *srcSpec,
+unsigned int flags
ATTRIBUTE_UNUSED)
...
+const char *prog[] = { SHOWMOUNT, --no-headers, --exports, NULL,
NULL };
+const char *start_tag = SourceList\n;
+const char *end_tag = /SourceList\n;
I'd prefer that to be sources - we avoid capitals in the
XML element names everywhere else, and in the few cases of
joining words use an underscore, but I think plural form is
OK for this.
+static char *
+virStorageBackendLogicalDiscoverPools(virConnectPtr conn,
+ const char *srcSpec ATTRIBUTE_UNUSED,
+ unsigned int flags ATTRIBUTE_UNUSED)
+const char *prog[] = { VGS, --noheadings, -o, vg_name, NULL };
+const char *start_tag = SourceList\n;
+const char *end_tag = /SourceList\n;
We should #define these two tags in storage_backend.h so each driver doesn't
need to dup them
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Fri, 2008-08-22 at 09:50 +0100, Daniel P. Berrange wrote: +const char *start_tag = SourceList\n; +const char *end_tag = /SourceList\n; I'd prefer that to be sources - we avoid capitals in the XML element names everywhere else, and in the few cases of joining words use an underscore, but I think plural form is OK for this. I prefer sources as well, so I'll change this. And I'll put those defines in storage_backend.h as well. As long as we're on the subject of naming (and before it's too late), it's been bothering me that we keep calling this storage pool discovery. To me, storage source discovery seems more accurate (because they're not pools until we define libvirt pools based on the sources). So I'd prefer renaming the various *Discover[Storage]Pools* functions (and support structs) introduced in this patch to *Discover[Storage]Sources*. I was just sticking with the originally-proposed names to avoid confusion. What do you all think? Dave -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
Here's the patch with those issues addressed (also merged with latest
upstream - avoids internal.h merge conflict) ...
Dave
On Fri, 2008-08-22 at 09:50 +0100, Daniel P. Berrange wrote:
On Thu, Aug 21, 2008 at 10:29:43AM -0400, David Lively wrote:
Hi Folks -
Here's my second pass at storage pool discovery. I've taken Daniel's
suggestion and made it return a single XML doc containing source
elements rather than an array of pool docs (and also incorporated
suggestions from Daniel V and Jim M).
Note that the storage source name patch is closely related
(without it, the source docs returned for logical pools aren't
correct).
Excellant, I think this patch looks more or less good to commit now.
+static char *
+virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
+const char *srcSpec,
+unsigned int flags
ATTRIBUTE_UNUSED)
...
+const char *prog[] = { SHOWMOUNT, --no-headers, --exports, NULL,
NULL };
+const char *start_tag = SourceList\n;
+const char *end_tag = /SourceList\n;
I'd prefer that to be sources - we avoid capitals in the
XML element names everywhere else, and in the few cases of
joining words use an underscore, but I think plural form is
OK for this.
+static char *
+virStorageBackendLogicalDiscoverPools(virConnectPtr conn,
+ const char *srcSpec ATTRIBUTE_UNUSED,
+ unsigned int flags ATTRIBUTE_UNUSED)
+const char *prog[] = { VGS, --noheadings, -o, vg_name, NULL };
+const char *start_tag = SourceList\n;
+const char *end_tag = /SourceList\n;
We should #define these two tags in storage_backend.h so each driver doesn't
need to dup them
Daniel
diff --git a/configure.in b/configure.in
index 9479f1c..430a097 100644
--- a/configure.in
+++ b/configure.in
@@ -671,6 +671,11 @@ 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])
+fi
AC_PATH_PROG([QEMU_IMG], [qemu-img], [], [$PATH:/sbin:/usr/sbin:/bin:/usr/bin])
if test -n $QEMU_IMG ; then
diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h
index 9c3e1c2..5308fa9 100644
--- a/include/libvirt/libvirt.h
+++ b/include/libvirt/libvirt.h
@@ -890,6 +890,14 @@ int virConnectListDefinedStoragePools(virConnectPtr conn,
int maxnames);
/*
+ * Query a host for storage pools of a particular type
+ */
+char * virConnectDiscoverStoragePools(virConnectPtr conn,
+ const char *type,
+ const char *srcSpec,
+ unsigned int flags);
+
+/*
* Lookup pool by name or uuid
*/
virStoragePoolPtr virStoragePoolLookupByName (virConnectPtr conn,
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index f077a26..3d06d76 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -890,6 +890,14 @@ int virConnectListDefinedStoragePools(virConnectPtr conn,
int maxnames);
/*
+ * Query a host for storage pools of a particular type
+ */
+char * virConnectDiscoverStoragePools(virConnectPtr conn,
+ const char *type,
+ const char *srcSpec,
+ unsigned int flags);
+
+/*
* Lookup pool by name or uuid
*/
virStoragePoolPtr virStoragePoolLookupByName (virConnectPtr conn,
diff --git a/qemud/remote.c b/qemud/remote.c
index b5a6ec9..43b3a56 100644
--- a/qemud/remote.c
+++ b/qemud/remote.c
@@ -2958,6 +2958,27 @@ 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);
+
+ret-xml =
+virConnectDiscoverStoragePools (client-conn,
+args-type,
+args-srcSpec ? *args-srcSpec : NULL,
+
Re: [libvirt] [PATCH] storage pool discovery
On Fri, Aug 22, 2008 at 10:58:54AM -0400, David Lively wrote: On Fri, 2008-08-22 at 09:50 +0100, Daniel P. Berrange wrote: +const char *start_tag = SourceList\n; +const char *end_tag = /SourceList\n; I'd prefer that to be sources - we avoid capitals in the XML element names everywhere else, and in the few cases of joining words use an underscore, but I think plural form is OK for this. I prefer sources as well, so I'll change this. And I'll put those defines in storage_backend.h as well. As long as we're on the subject of naming (and before it's too late), it's been bothering me that we keep calling this storage pool discovery. To me, storage source discovery seems more accurate (because they're not pools until we define libvirt pools based on the sources). So I'd prefer renaming the various *Discover[Storage]Pools* functions (and support structs) introduced in this patch to *Discover[Storage]Sources*. I was just sticking with the originally-proposed names to avoid confusion. What do you all think? That sounds like a reasonable idea to me. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
David Lively [EMAIL PROTECTED] wrote:
Here's the patch with those issues addressed (also merged with latest
upstream - avoids internal.h merge conflict) ...
Hi David,
Looks good.
A couple of minor suggestions and questions.
...
diff --git a/configure.in b/configure.in
index 9479f1c..430a097 100644
--- a/configure.in
+++ b/configure.in
@@ -671,6 +671,11 @@ 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])
+fi
Do we want to require the showmount package via the spec file?
...
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;
path can be const, too.
+
+path = groups[0];
+
+name = strrchr(path, '/');
+if (name == NULL) {
+virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _(invalid netfs path (no slash): %s), path);
+return -1;
+}
+name += 1;
It'd be good to diagnose a path that ends in a slash (*name == '\0'),
rather than emitting XML with an empty name, below.
+/* Append new XML desc to list */
+
+if (VIR_ALLOC(newItem) != 0) {
+virStorageReportError(conn, VIR_ERR_NO_MEMORY, %s, _(new xml
desc));
+return -1;
+}
+
+if (asprintf(newItem-val,
+ sourcehost name='%s'/dir path='%s'//source\n,
+ state-host, path) = 0) {
+virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, %s,
_(asprintf failed));
+VIR_FREE(newItem);
+return -1;
+}
+
+newItem-len = strlen(newItem-val);
+newItem-next = state-list;
+state-list = newItem;
+
+return 0;
+}
+
+static char *
+virStorageBackendFileSystemNetDiscoverPools(virConnectPtr conn,
+const char *srcSpec,
+unsigned int flags
ATTRIBUTE_UNUSED)
+{
+/*
+ * # 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) +\\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, len;
Please use size_t as the type for string-length
variables like len.
+virStringList *p;
+char *retval = NULL;
+
+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;
+}
+
+xpath_ctxt = xmlXPathNewContext(doc);
+if (xpath_ctxt == NULL) {
+virStorageReportError(conn, VIR_ERR_NO_MEMORY, %s,
_(xpath_ctxt));
+goto cleanup;
+}
+
+state.host = virXPathString(conn, string(/source/host/@name),
xpath_ctxt);
+if (!state.host || !state.host[0]) {
+virStorageReportError(conn, VIR_ERR_XML_ERROR, %s,
+ _(missing host in source spec));
+goto cleanup;
+}
+prog[3] = state.host;
+
+if (virStorageBackendRunProgRegex(conn, NULL, prog, 1, regexes, vars,
+
virStorageBackendFileSystemNetDiscoverPoolsFunc,
+ state, exitstatus) 0)
+goto cleanup;
+
+/* Turn list of strings into final document */
+len = strlen(SOURCES_START_TAG) + strlen(SOURCES_END_TAG);
+for (p = state.list; p; p = p-next)
+len += p-len;
+if (VIR_ALLOC_N(retval, len+1) 0) {
+virStorageReportError(conn, VIR_ERR_NO_MEMORY, _(retval (%d
bytes)), len);
+goto cleanup;
+}
+strcpy(retval, SOURCES_START_TAG);
+len = strlen(SOURCES_START_TAG);
+for (p = state.list; p; p = p-next) {
+strcpy(retval + len, p-val);
+len += p-len;
+}
+strcpy(retval + len, SOURCES_END_TAG);
+
+ cleanup:
+
Re: [libvirt] [PATCH] storage pool discovery
Hi Jim - Thanks for your comments. I really appreciate the detailed look. I'll implement your suggestions (including the refactoring of the code to turn a virStringList into a single XML string), though I have a question about one of them: On Fri, 2008-08-22 at 19:16 +0200, Jim Meyering wrote: diff --git a/configure.in b/configure.in index 9479f1c..430a097 100644 --- a/configure.in +++ b/configure.in @@ -671,6 +671,11 @@ 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]) +fi Do we want to require the showmount package via the spec file? I was a little worried about this too. It's probably better to handle this more like iscsiadmin/--with-storage-iscsi: defaults to enabling iscsi backend if and only if iscsiadmin is present. Fails with --with-storage-iscsi=yes explicitly specified and iscsiadmin not found. But I'm not sure we want to disable the storage_fs backend just because we can't find showmount. So maybe there should be another config option --with-storage-netfs-discovery to cover the only code that actually uses showmount? Either of these sounds reasonable to me. Do you have a preference? Dave -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Fri, 2008-08-22 at 19:16 +0200, Jim Meyering wrote:
+const char *name, *path;
path can be const, too.
It is, at least according to my gcc (4.3.0-8, x86_64, Fedora9).
Compile the following and without FORCE_WARNING to check:
void check()
{
#ifdef FORCE_WARNING
const char *foo;
char *bar;
#else
const char *foo, *bar;
#endif
foo = I'm a const char *;
bar = foo; // warns iff bar not const char *
}
Dave
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
David Lively [EMAIL PROTECTED] wrote:
On Fri, 2008-08-22 at 19:16 +0200, Jim Meyering wrote:
+const char *name, *path;
path can be const, too.
It is, at least according to my gcc (4.3.0-8, x86_64, Fedora9).
Compile the following and without FORCE_WARNING to check:
void check()
{
#ifdef FORCE_WARNING
const char *foo;
char *bar;
#else
const char *foo, *bar;
#endif
Oops. Now you know why I prefer to declare them on separate lines:
one less rule to remember that way ;-)
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
David Lively [EMAIL PROTECTED] wrote: ... 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]) +fi Do we want to require the showmount package via the spec file? I was a little worried about this too. It's probably better to handle this more like iscsiadmin/--with-storage-iscsi: defaults to enabling iscsi backend if and only if iscsiadmin is present. Fails with --with-storage-iscsi=yes explicitly specified and iscsiadmin not found. Adding Requires: showmount and BuildRequires lines to libvirt.spec.in would be similar to the approach of requiring the iscsi-initiator-utils package (which contains iscsiadm): $ grep isc *.spec.in Requires: iscsi-initiator-utils BuildRequires: iscsi-initiator-utils -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Fri, Aug 22, 2008 at 11:06:38PM +0200, Jim Meyering wrote: David Lively [EMAIL PROTECTED] wrote: ... 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]) +fi Do we want to require the showmount package via the spec file? I was a little worried about this too. It's probably better to handle this more like iscsiadmin/--with-storage-iscsi: defaults to enabling iscsi backend if and only if iscsiadmin is present. Fails with --with-storage-iscsi=yes explicitly specified and iscsiadmin not found. Adding Requires: showmount and BuildRequires lines to libvirt.spec.in would be similar to the approach of requiring the iscsi-initiator-utils package (which contains iscsiadm): Yes, basically the spec file should encode 'best practice' and as such it is reasonable to add a 'Requires: showmount'. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Fri, 2008-08-01 at 10:40 +0100, Daniel P. Berrange wrote: On Wed, Jul 30, 2008 at 04:30:01PM -0400, David Lively wrote: 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 targetpath 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. This is really two suggestions rolled into one: (1) just return source xml (not whole pool xml), and (2) instead of returning an array of strings, return a single (xml) string wrapped with a poolList element Either (1) or (2) could be done independently of the other, and I think they're both worth considering. I like the fact that (2) lets us auto-generate the python binding, but I don't have strong feelings either way. I like (1) a lot, but it doesn't really work correctly for logical storage pools in their current incarnation. The source element for a logical pool currently consists only of the device paths to the physical volumes backing the volume group. In particular, it doesn't specify the volume group name (which seems to be assumed to be the same as the libvirt pool name??). This could be fixed by allowing the source element for an existing volume group to specify (only) the volume group name (which might be different from the pool name). In this way, logical pool discovery can return source elements that fully specify existing pools. And the client app can decide on pool names and target paths as you propose. [Note I'm proposing *extending* logical pool creation, so existing XML should continue to work. Specifically I'm proposing we extend source with an optional name element which the logical storage backend will interpret as volume group name. For back-compatibility, the backend will default the source name to the pool name when the source name isn't specified. The source name is used to instantiate existing volume groups as well as to name new volume groups (whose source must also specify the backing physical devices).] Does this sound reasonable? 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... Sorry, I'm an emacs guy :-) [What we *really* need is an emacs mode (indent-assimilation-mode) that will figure out the indentation settings for each file from the code that's already there ... (assuming it's consistent ...). I suppose it wouldn't be that hard to at least get c-basic-offset, c-indent-level, and indent-tabs-mode right. (I know you can embed emacs directives in source comments, but that's kind of ugly ...)] In any case, thanks much for your comments. I'm finally getting back to working on this, so I should be able to submit my next take soon. Thanks, Dave -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
Thanks much for your comments, Jim. They all look reasonable (though I
may be intentionally trimming a NL in one of these cases -- I'm
checking).
And I'll start following the HACKING file recommendations before
submitting my next attempt :-)
(Though note I'm not yet submitting tests since we haven't really
finished hashing out the API - at least some crucial XML details ...)
Dave
On Mon, 2008-08-04 at 08:25 +0200, Jim Meyering wrote:
Hi David,
I spotted a few things that would be good to change:
David Lively [EMAIL PROTECTED] 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
Re: [libvirt] [PATCH] storage pool discovery
Hi David,
I spotted a few things that would be good to change:
David Lively [EMAIL PROTECTED] 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 /.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Mon, Aug 04, 2008 at 11:02:33AM +0200, Stefan de Konink wrote: On Mon, 4 Aug 2008, Daniel P. Berrange wrote: - - For iSCSI and related stuff everything was relatively easy, because this would just mean to write the right /dev/blabla to the xenstore. What is your idea to get different drivers working via: virt://pool/volume (so basically blktap vs file vs disk) My idea was to have a script in /etc/xen/scripts/ Me too, but in order to 'fetch' the actual configuration it is required to contact libvirt. And query about the pool/volume location. In this way it would be actually a 'redirection' to blktap or adding a devicepath. So this script is now written in plain C, but I want to know how you imagine the driver selection based on connection uri. You can simply use xen:/// as the URI. There is no need for configurable URIs since thisis a xen specific script. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Mon, Aug 04, 2008 at 11:19:25AM +0200, Stefan de Konink wrote: Daniel P. Berrange schreef: On Mon, Aug 04, 2008 at 11:02:33AM +0200, Stefan de Konink wrote: On Mon, 4 Aug 2008, Daniel P. Berrange wrote: - - For iSCSI and related stuff everything was relatively easy, because this would just mean to write the right /dev/blabla to the xenstore. What is your idea to get different drivers working via: virt://pool/volume (so basically blktap vs file vs disk) My idea was to have a script in /etc/xen/scripts/ Me too, but in order to 'fetch' the actual configuration it is required to contact libvirt. And query about the pool/volume location. In this way it would be actually a 'redirection' to blktap or adding a devicepath. So this script is now written in plain C, but I want to know how you imagine the driver selection based on connection uri. You can simply use xen:/// as the URI. There is no need for configurable URIs since thisis a xen specific script. You don't get the issue. In order to run a specific script for example a block device, it should have an unique prefix. That will make the executable that is called so virt:// will call /etc/xen/scripts/block-virt as script. As you might notice here, the common file://, tap:aio:// or psy:// is not present, so if a pool is file based or device based it should some how inform this 'script' how to redirect the parameters to the appropriate script. This is what the driver tag in the XML can be used for. It can include a driver name (file, phys, tap) and subtype (raw, aio, qcow, etc). You just need to decide how to encode this info when passing it to Xend. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
Daniel P. Berrange schreef: On Mon, Aug 04, 2008 at 11:19:25AM +0200, Stefan de Konink wrote: Daniel P. Berrange schreef: On Mon, Aug 04, 2008 at 11:02:33AM +0200, Stefan de Konink wrote: On Mon, 4 Aug 2008, Daniel P. Berrange wrote: - - For iSCSI and related stuff everything was relatively easy, because this would just mean to write the right /dev/blabla to the xenstore. What is your idea to get different drivers working via: virt://pool/volume (so basically blktap vs file vs disk) My idea was to have a script in /etc/xen/scripts/ Me too, but in order to 'fetch' the actual configuration it is required to contact libvirt. And query about the pool/volume location. In this way it would be actually a 'redirection' to blktap or adding a devicepath. So this script is now written in plain C, but I want to know how you imagine the driver selection based on connection uri. You can simply use xen:/// as the URI. There is no need for configurable URIs since thisis a xen specific script. You don't get the issue. In order to run a specific script for example a block device, it should have an unique prefix. That will make the executable that is called so virt:// will call /etc/xen/scripts/block-virt as script. As you might notice here, the common file://, tap:aio:// or psy:// is not present, so if a pool is file based or device based it should some how inform this 'script' how to redirect the parameters to the appropriate script. This is what the driver tag in the XML can be used for. It can include a driver name (file, phys, tap) and subtype (raw, aio, qcow, etc). You just need to decide how to encode this info when passing it to Xend. Yes... and this was exactly my question ;) So if we have a pool and want to send it to xen it would look something like: virt://driver/pool/volume, now the 'tiny' problem is that I don't want to reimplement every possible driver. So I'll do some research in order to pass a connection url exported by libvirt to the 'default' scripts, while maintaining the virt:// in xen. Stefan -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
Daniel P. Berrange schreef: On Mon, Aug 04, 2008 at 11:02:33AM +0200, Stefan de Konink wrote: On Mon, 4 Aug 2008, Daniel P. Berrange wrote: - - For iSCSI and related stuff everything was relatively easy, because this would just mean to write the right /dev/blabla to the xenstore. What is your idea to get different drivers working via: virt://pool/volume (so basically blktap vs file vs disk) My idea was to have a script in /etc/xen/scripts/ Me too, but in order to 'fetch' the actual configuration it is required to contact libvirt. And query about the pool/volume location. In this way it would be actually a 'redirection' to blktap or adding a devicepath. So this script is now written in plain C, but I want to know how you imagine the driver selection based on connection uri. You can simply use xen:/// as the URI. There is no need for configurable URIs since thisis a xen specific script. You don't get the issue. In order to run a specific script for example a block device, it should have an unique prefix. That will make the executable that is called so virt:// will call /etc/xen/scripts/block-virt as script. As you might notice here, the common file://, tap:aio:// or psy:// is not present, so if a pool is file based or device based it should some how inform this 'script' how to redirect the parameters to the appropriate script. Stefan -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Sun, Aug 03, 2008 at 04:20:50AM +0200, Stefan de Konink wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Daniel, Stefan de Konink schreef: As you know I wrote such thing for iSCSI/Solaris, iSCSI/Netapp so... if you want such script I can fabricate something, maybe it would be interesting to not create a script, but instead a binary that links to libvirt and is able to lookup the connection url and creates the xenstore stuff. I have some code now that basically parses the XENBUS_PATH/param and looks it up in libvirt. Now I have some questions: - - Is it possible to reuse the hypervisor connection from within libvirt? I would like to make this working: xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn-privateData; You are only allowed to cast to xenUnifiedPrivatePtr if you are running within the Xen driver code. It is forbidden in all other places. - - For iSCSI and related stuff everything was relatively easy, because this would just mean to write the right /dev/blabla to the xenstore. What is your idea to get different drivers working via: virt://pool/volume (so basically blktap vs file vs disk) My idea was to have a script in /etc/xen/scripts/ 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Mon, 4 Aug 2008, Daniel P. Berrange wrote: - - For iSCSI and related stuff everything was relatively easy, because this would just mean to write the right /dev/blabla to the xenstore. What is your idea to get different drivers working via: virt://pool/volume (so basically blktap vs file vs disk) My idea was to have a script in /etc/xen/scripts/ Me too, but in order to 'fetch' the actual configuration it is required to contact libvirt. And query about the pool/volume location. In this way it would be actually a 'redirection' to blktap or adding a devicepath. So this script is now written in plain C, but I want to know how you imagine the driver selection based on connection uri. Stefan -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Daniel, Stefan de Konink schreef: As you know I wrote such thing for iSCSI/Solaris, iSCSI/Netapp so... if you want such script I can fabricate something, maybe it would be interesting to not create a script, but instead a binary that links to libvirt and is able to lookup the connection url and creates the xenstore stuff. I have some code now that basically parses the XENBUS_PATH/param and looks it up in libvirt. Now I have some questions: - - Is it possible to reuse the hypervisor connection from within libvirt? I would like to make this working: xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn-privateData; - - For iSCSI and related stuff everything was relatively easy, because this would just mean to write the right /dev/blabla to the xenstore. What is your idea to get different drivers working via: virt://pool/volume (so basically blktap vs file vs disk) Stefan -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEAREKAAYFAkiVFgIACgkQYH1+F2Rqwn2RcgCfQCXeZA6OTqGsoWYMH6yW20dy dL4AnAtilh4ezQLtyulGeuoAt4XXrCJ4 =6S5u -END PGP SIGNATURE- -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
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 sourcehost 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 targetpath 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 = (
Re: [libvirt] [PATCH] storage pool discovery
Daniel, When this gets in; would it get more easy to lookup a file-storagepool. In order to make the alternative configuration I proposed for domains working? (Basically providing pool/volume instead of filename) Stefan -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Fri, Aug 01, 2008 at 11:46:14AM +0200, Stefan de Konink wrote: Daniel, When this gets in; would it get more easy to lookup a file-storagepool. In order to make the alternative configuration I proposed for domains working? (Basically providing pool/volume instead of filename) I don't think it'll make any significant difference - we can do the alternate XML syntax you proposed already. I think it might be nice to use a different approach to the Xen impl of it though. Previously have had done the poool/volume - filename conversion in libvirt before passing the data to XenD. This means that when you don't the XML you get the filename instead of pool/volume details, and doesn't deal with potentially different filenames upon migration. So I'd like to see if its possible to write a custom script for xen to go in /etc/xen/scripts. So we can pass in a pool/volume to Xend and have it auto-lookup the file at time of hotplug. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage pool discovery
On Fri, 1 Aug 2008, Daniel P. Berrange wrote: On Fri, Aug 01, 2008 at 11:46:14AM +0200, Stefan de Konink wrote: Daniel, When this gets in; would it get more easy to lookup a file-storagepool. In order to make the alternative configuration I proposed for domains working? (Basically providing pool/volume instead of filename) I don't think it'll make any significant difference - we can do the alternate XML syntax you proposed already. I think it might be nice to use a different approach to the Xen impl of it though. Previously have had done the poool/volume - filename conversion in libvirt before passing the data to XenD. This means that when you don't the XML you get the filename instead of pool/volume details, and doesn't deal with potentially different filenames upon migration. So I'd like to see if its possible to write a custom script for xen to go in /etc/xen/scripts. So we can pass in a pool/volume to Xend and have it auto-lookup the file at time of hotplug. As you know I wrote such thing for iSCSI/Solaris, iSCSI/Netapp so... if you want such script I can fabricate something, maybe it would be interesting to not create a script, but instead a binary that links to libvirt and is able to lookup the connection url and creates the xenstore stuff. But as you look at it now, this gets very close to what XenD is doing, so if we implement it in this way, why not do everything that is required ;) Stefan -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
