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 <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.
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