[Libvir] PATCH: 1/16: public API

[Daniel P. Berrange]

On Thu, Feb 14, 2008 at 06:06:08AM -0500, Daniel Veillard wrote:
> On Tue, Feb 12, 2008 at 04:30:07AM +0000, Daniel P. Berrange wrote:
> > +
> > +
> > +typedef enum {
> > +  VIR_STORAGE_POOL_BUILD_NEW  = 0,   /* Regular build from scratch */
> > +  VIR_STORAGE_POOL_BUILD_REPAIR = 1, /* Repair / reinitialize */
> > +  VIR_STORAGE_POOL_BUILD_EXTEND = 2  /* Extend existing pool */
> 
>   Is shrinking not possible ? things like compressing to fewer volumes

I guess it could conceivably be supported. I should rename this to
be VIR_STORAGE_POOL_BUILD_RESIZE  since the distinction between extending
and shrinking is not really relevant from the context of this API.

> > +} virStoragePoolBuildFlags;
> > +
> > +typedef enum {
> > +  VIR_STORAGE_POOL_DELETE_NORMAL = 0, /* Delete metadata only    (fast) */
> > +  VIR_STORAGE_POOL_DELETE_CLEAR = 1,  /* Clear all data to zeros (slow) */
> 
>   I would name it VIR_STORAGE_POOL_DELETE_ZEROED , it's more obvious what
> the operstion does.

Good idea.

> > +typedef enum {
> > +  VIR_STORAGE_VOL_FILE = 0,     /* Regular file based volumes */
> > +  VIR_STORAGE_VOL_BLOCK = 1,    /* Block based volumes */
> > +  VIR_STORAGE_VOL_VIRTUAL = 2,  /* Volumes which aren't assignable to guests */
> > +} virStorageVolType;
> 
>   I wonder if it's worth making the distinction between virtual storage with
> local state and those without, the second having really good properties for
> example when deciding to do  a migration. But current enum is fine already.

Sorry, there's a mistake here - the VIR_STORAGE_VOL_VIRTUAL should not be
there anymore. Since I removed the concept of 'singleton pools' it is no
longer neccessary - previously it would be used for a volume which pointed
to a LVM volume group - but this was just stupid, so i killed it.

> > +typedef enum {
> > +  VIR_STORAGE_VOL_DELETE_NORMAL = 0, /* Delete metadata only    (fast) */
> > +  VIR_STORAGE_VOL_DELETE_CLEAR = 1,  /* Clear all data to zeros (slow) */
> > +} virStorageVolDeleteFlags;
> 
>   I still prefer ZEROED as CLEAr but not a big deal.

Yep, I like it too

> > +virStoragePoolPtr	virStoragePoolCreateXML		(virConnectPtr conn,
> > +							 const char *xmlDesc);
> > +virStoragePoolPtr	virStoragePoolDefineXML		(virConnectPtr conn,
> > +							 const char *xmlDesc);
> > +int			virStoragePoolBuild		(virStoragePoolPtr pool,
> > +							 unsigned int flags);
> > +int			virStoragePoolUndefine		(virStoragePoolPtr pool);
> > +int			virStoragePoolCreate		(virStoragePoolPtr pool);
> 
> 
>   Can we use a flag here ? Is the operation synchronous or not ?
> Or is that an instant operation and only Build() is likely to take some time.
> I would add an unused flag for safety here.

Definitely. I should have a flags paramater on all 'Create' methods since they can
take non-trivial time - eg to login to the remote server, and we might wish to 
control the behaviour via flags.

>   Okay, +1, 
> I would love to see this or part of it commited before the next 
> round, even if all entry points are present, as a temporary way
> to decrease patches and separate the agreed upon from what would need 
> to be refined. I guess it's not a problem until we make a new release.

That works with me - it is non-triival work keeping them in sync, even with
excellant quilt/mq tools.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|