9:29 a.m.
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).
Dave
3:50 a.m.
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 :|
9:58 a.m.
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
10:15 a.m.
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 :|
9:33 a.m.
On Fri, 2008-08-22 at 16:15 +0100, Daniel P. Berrange wrote:
On Fri, Aug 22, 2008 at 10:58:54AM -0400, David Lively wrote:
> 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.
[Sorry to harp on the naming issue. But names are important, and we
can't change them once they're in the API ...]
After making the change I suggested above, it now feels a little strange
because "Pool" is gone from the name. I'm starting to think
"*Discover[Storage]PoolSources*" is the only good choice. It's rather
long, but makes it clear we're talking about storage pool sources (as
opposed to "storage sources", which feels a little ambiguous, or
"storage pools" which isn't accurate since they're not (yet) pools).
Sound ok?
Dave
11:51 a.m.
On Mon, Aug 25, 2008 at 10:33:45AM -0400, David Lively wrote:
On Fri, 2008-08-22 at 16:15 +0100, Daniel P. Berrange wrote:
> On Fri, Aug 22, 2008 at 10:58:54AM -0400, David Lively wrote:
> > 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.
[Sorry to harp on the naming issue. But names are important, and we
can't change them once they're in the API ...]
After making the change I suggested above, it now feels a little strange
because "Pool" is gone from the name. I'm starting to think
"*Discover[Storage]PoolSources*" is the only good choice. It's rather
long, but makes it clear we're talking about storage pool sources (as
opposed to "storage sources", which feels a little ambiguous, or
"storage pools" which isn't accurate since they're not (yet) pools).
Discover is a bit of a long word - lets use 'Find' instead, so it makes
the API name a little shorter - and no worse than our existing longest
API name. In other words:
virConnectFindStoragePoolSources()
Regards,
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 :|
2:34 p.m.
On Wed, 2008-08-27 at 17:51 +0100, Daniel P. Berrange wrote:
On Mon, Aug 25, 2008 at 10:33:45AM -0400, David Lively wrote:
> After making the change I suggested above, it now feels a little strange
> because "Pool" is gone from the name. I'm starting to think
> "*Discover[Storage]PoolSources*" is the only good choice. It's
rather
> long, but makes it clear we're talking about storage pool sources (as
> opposed to "storage sources", which feels a little ambiguous, or
> "storage pools" which isn't accurate since they're not (yet)
pools).
Discover is a bit of a long word - lets use 'Find' instead, so it makes
the API name a little shorter - and no worse than our existing longest
API name. In other words:
virConnectFindStoragePoolSources()
Sounds good. The attached patch uses this name (and others consistent
with it), and should implement all the suggestions from Daniel V and Jim
M.
Thanks again to all three of you for the helpful comments.
Dave
3:06 p.m.
On Wed, Aug 27, 2008 at 03:34:14PM -0400, David Lively wrote:
On Wed, 2008-08-27 at 17:51 +0100, Daniel P. Berrange wrote:
> On Mon, Aug 25, 2008 at 10:33:45AM -0400, David Lively wrote:
> > After making the change I suggested above, it now feels a little strange
> > because "Pool" is gone from the name. I'm starting to think
> > "*Discover[Storage]PoolSources*" is the only good choice. It's
rather
> > long, but makes it clear we're talking about storage pool sources (as
> > opposed to "storage sources", which feels a little ambiguous, or
> > "storage pools" which isn't accurate since they're not (yet)
pools).
>
> Discover is a bit of a long word - lets use 'Find' instead, so it makes
> the API name a little shorter - and no worse than our existing longest
> API name. In other words:
>
> virConnectFindStoragePoolSources()
Sounds good. The attached patch uses this name (and others consistent
with it), and should implement all the suggestions from Daniel V and Jim
M.
Excellant work - I've committed all this to CVS before we change our
minds on naming yet again :-)
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 :|
10:11 a.m.
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
12:16 p.m.
David Lively <dlively(a)virtualiron.com> 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,
+ "<source><host 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:
+ xmlFreeDoc(doc);
+ xmlXPathFreeContext(xpath_ctxt);
+ VIR_FREE(state.host);
+ while (state.list) {
+ p = state.list->next;
+ VIR_FREE(state.list);
+ state.list = p;
+ }
+
+ return retval;
+}
...
diff --git a/src/storage_backend_logical.c
b/src/storage_backend_logical.c
...
+static char *
+virStorageBackendLogicalDiscoverPools(virConnectPtr conn,
+ const char *srcSpec ATTRIBUTE_UNUSED,
+ unsigned int flags ATTRIBUTE_UNUSED)
+{
+ /*
+ * # sudo vgs --noheadings -o vg_name
+ * VolGroup00
+ * VolGroup01
+ */
+ const char *regexes[] = {
+ "^\\s*(\\S+)\\s*$"
+ };
+ int vars[] = {
This can be const.
+ 1
+ };
+ virStringList *descs = NULL;
+ const char *prog[] = { VGS, "--noheadings", "-o",
"vg_name", NULL };
+ int exitstatus, len;
len should be size_t, here too.
+ virStringList *p;
+ char *retval = NULL;
+
+ if (virStorageBackendRunProgRegex(conn, NULL, prog, 1, regexes, vars,
+ virStorageBackendLogicalDiscoverPoolsFunc,
+ &descs, &exitstatus) < 0)
+ return NULL;
+
The following block of 15 lines is identical to the one above.
It'd be nice to factor out the duplication.
Actually, this
virStorageBackendLogicalDiscoverPools and the preceding
virStorageBackendFileSystemNetDiscoverPools share enough
code that I wonder if it'd might be worthwhile to unify them.
Probably not, but it's close...
+ /* Turn list of strings into final document */
+ len = strlen(SOURCES_START_TAG) + strlen(SOURCES_END_TAG);
+ for (p = descs; 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 = descs; p; p = p->next) {
+ strcpy(retval + len, p->val);
+ len += p->len;
+ }
+ strcpy(retval + len, SOURCES_END_TAG);
+
+ cleanup:
+ while (descs) {
+ p = descs->next;
+ VIR_FREE(descs);
+ descs = p;
+ }
+
+ return retval;
+}
...
diff --git a/src/virsh.c b/src/virsh.c
index 67d9658..08fb425 100644
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -3422,6 +3422,138 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
return TRUE;
}
+/*
+ * "pool-discover-as" command
+ */
+static vshCmdInfo info_pool_discover_as[] = {
I think all similar static array declarations are "const".
At least they were supposed to be.
We probably need an automatic check for this.
static const vshCmdInfo info_pool_discover_as[] = {
+ {"syntax", "pool-discover-as <type>
[options]"},
+ {"help", gettext_noop("discover pools")},
+ {"desc", gettext_noop("Returns discover of pools.")},
+ {NULL, NULL}
+};
+
+static vshCmdOptDef opts_pool_discover_as[] = {
+ {"type", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("type of storage
pool to discover")},
+ {"host", VSH_OT_DATA, VSH_OFLAG_NONE, gettext_noop("optional host to
query")},
+ {"port", VSH_OT_DATA, VSH_OFLAG_NONE, gettext_noop("optional port to
query")},
+ {NULL, 0, 0, NULL}
+};
+
+static int
+cmdPoolDiscoverAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED)
+{
+ char *type, *host;
+ char *srcSpec = NULL;
+ char *srcList;
+ int found;
+
+ type = vshCommandOptString(cmd, "type", &found);
+ if (!found)
+ return FALSE;
+ host = vshCommandOptString(cmd, "host", &found);
+ if (!found)
+ host = NULL;
+
+ if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
+ return FALSE;
+
+ if (host) {
+ int hostlen = strlen(host);
use size_t
+ char *port = vshCommandOptString(cmd, "port",
&found);
+ int ret;
+ if (!found) {
+ port = strrchr(host, ':');
+ if (port) {
+ if (*(++port))
+ hostlen = (int)(port - host - 1);
no need for the cast to "int"
+ else
+ port = NULL;
+ }
+ }
+ ret = port ?
+ asprintf(&srcSpec,
+ "<source><host name='%.*s'
port='%s'/></source>",
+ hostlen, host, port) :
Do the hostname and port string need to be quoted (and/or evoke
warning/failure), in case they contain things like "'"?
+ asprintf(&srcSpec,
+ "<source><host
name='%.*s'/></source>",
+ hostlen, host);
+ if (ret < 0) {
+ switch (errno) {
+ case ENOMEM:
+ vshError(ctl, FALSE, "%s", _("Out of memory"));
+ break;
+ default:
+ vshError(ctl, FALSE, _("asprintf failed (errno %d)"), errno);
+ }
+ return FALSE;
+ }
+ }
+
+ srcList = virConnectDiscoverStoragePools(ctl->conn, type, srcSpec, 0);
+ free(srcSpec);
+ if (srcList == NULL) {
+ vshError(ctl, FALSE, "%s", _("Failed to discover pools"));
+ return FALSE;
+ }
+ vshPrint(ctl, "%s", srcList);
+ free(srcList);
+
+ return TRUE;
+}
+
+
+/*
+ * "pool-discover" command
+ */
+static vshCmdInfo info_pool_discover[] = {
If these can be const, please make them so (along with any above):
static const vshCmdInfo info_pool_discover[] = {
+ {"syntax", "pool-discover <type>
[srcSpec]"},
+ {"help", gettext_noop("discover pools")},
+ {"desc", gettext_noop("Returns discover of pools.")},
+ {NULL, NULL}
+};
+
+static vshCmdOptDef opts_pool_discover[] = {
+ {"type", VSH_OT_DATA, VSH_OFLAG_REQ,
+ gettext_noop("type of storage pool to discover")},
+ {"srcSpec", VSH_OT_DATA, VSH_OFLAG_NONE,
+ gettext_noop("optional file of source xml to query for pools")},
+ {NULL, 0, 0, NULL}
+};
...
2:53 p.m.
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
4:06 p.m.
David Lively <dlively(a)virtualiron.com> 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
4:08 p.m.
On Fri, Aug 22, 2008 at 11:06:38PM +0200, Jim Meyering wrote:
David Lively <dlively(a)virtualiron.com> 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 :|
3:03 p.m.
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
3:56 p.m.
David Lively <dlively(a)virtualiron.com> 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 ;-)
3:31 p.m.
On Fri, 2008-08-22 at 19:16 +0200, Jim Meyering wrote:
> + asprintf(&srcSpec,
> + "<source><host name='%.*s'
port='%s'/></source>",
> + hostlen, host, port) :
Do the hostname and port string need to be quoted (and/or evoke
warning/failure), in case they contain things like "'"?
The host string isn't quoted when creating XML in some similar code in
cmdPoolCreateAs ... but that's not necessarily a good justification.
Is there an existing routine to quote a string to make it suitable for
an XML attribute value? (Something in libxml2, perhaps??) I'm not even
sure of the correct syntax to quote a "'" in an attribute value, for
example.
Dave
5931
days inactive
5937
days old
15 comments
3 participants
participants (3)
-
Daniel P. Berrange -
David Lively -
Jim Meyering