[libvirt] [PATCH 2/8] Basic framework for lock manager plugins

[Daniel P. Berrange]

On Tue, Nov 23, 2010 at 05:00:05PM -0500, David Teigland wrote:
> > +/**
> > + * virLockManagerNew:
> > + * @man: the lock manager context
> > + * @type: the type of process to be supervised
> > + * @flags: optional flags, currently unused
> > + *
> > + * Initialize a new context to supervise a process, usually
> > + * a virtual machine. If the lock driver requested a
> > + * private data, the <code>privateData</code> field of
> > + * <code>man</code> will be initialized with a suitable
> > + * pointer.
> > + *
> > + * Returns 0 if successful initialized a new context, -1 on error
> > + */
> > +typedef int (*virLockDriverNew)(virLockManagerPtr man,
> > +                                unsigned int type,
> > +                                unsigned int flags);
> 
> So I don't malloc man->privateData...

Nope, no need to malloc it. Just set the 'privateDataLen'
in the driver table to however many bytes you need, eg
sizeof(your private struct), and this will be malloced
at the same time as the main virLockManagerPtr in one
contigous block.

> 
> > +/**
> > + * virLockDriverFree:
> > + * @manager: the lock manager context
> > + *
> > + * Release any resources associated with the lock manager
> > + * context private data
> > + */
> > +typedef void (*virLockDriverFree)(virLockManagerPtr man);
> 
> but I do free it?

Nope, only need to free stuff stored in your private
data struct, not the private data struct itself.

> 
> > +/**
> > + * virLockDriverSetParameter:
> > + * @manager: the lock manager context
> > + * @key: the parameter name
> > + * @value: the parameter value
> > + *
> > + * Set a configuration parameter for the managed process.
> > + * A process of VIR_LOCK_MANAGER_START_DOMAIN will be
> > + * given at least 3 parameters:
> > + *
> > + * - id: the domain unique id
> > + * - uuid: the domain uuid
> > + * - name: the domain name
> > + *
> > + * There may be other parameters specific to the lock manager
> > + * plugin that are provided for the managed process
> > + *
> > + * This should only be called prior to the supervised process
> > + * being started.
> > + *
> > + * Returns 0 on success, or -1 on failure
> > + */
> > +typedef int (*virLockDriverSetParameter)(virLockManagerPtr man,
> > +                                         const char *key,
> > +                                         const char *value);
> 
> I think we may want to set parameters on resources as well.

What for ? I'd prefer to avoid that becuase I don't want to
create an intermediate object to fully represent resources

> > +/**
> > + * virLockDriverAcquireObject:
> > + * @manager: the lock manager context
> > + * @state: the current lock state
> > + * @flags: optional flags, currently unused
> > + *
> > + * Start managing resources for the object. If the
> > + * object is being transferred from another location
> > + * the current lock state may be passed in. This
> > + * must be called from the PID that represents the
> > + * object to be managed. If the lock is lost at any
> > + * time, the PID will be killed off by the lock manager.
> > + *
> > + * Returns 0 on success, or -1 on failure
> > + */
> > +typedef int (*virLockDriverAcquireObject)(virLockManagerPtr man,
> > +                                          const char *state,
> > +                                          unsigned int flags);
> 
> I know that the migration part is still pending, but I assume one of the
> flags here would indicate "incoming"?

In the case of incoming migration, 'state' will be non-NULL
ie a representation of the current lock state from the
migration source.

> 
> > +/**
> > + * virLockDriverGetState:
> > + * @manager: the lock manager context
> > + * @state: pointer to be filled with lock state
> > + * @flags: optional flags, currently unused
> > + *
> > + * Retrieve the current lock state. The returned
> > + * lock state may be NULL if none is required. The
> > + * caller is responsible for freeing the lock
> > + * state string when it is no longer required
> > + *
> > + * Returns 0 on success, or -1 on failure.
> > + */
> > +typedef int (*virLockDriverGetState)(virLockManagerPtr man,
> > +                                     char **state,
> > +                                     unsigned int flags);
> 
> Is there a reason that "state" should be a string instead of binary data
> with size?
> 
> This may have more uses than migration, but for migration it may be nice
> to return the state from the first migrate function on the source.

The 'state' will ultimately be placed in an XML document
to be transferred during the migration process. This API
only rewquires that it be a NULL terminated string. If
you need to transfer binary data as the state, then I'd
recommend just base64 encoding your blob of data. libvirt
will take care of any XML special character escaping if
it is needed.

> 
> > +/**
> > + * virLockDriverAcquireResource:
> > + * @manager: the lock manager context
> > + * @type: the resource type virLockDriverResourceType
> > + * @name: the resource name
> > + * @flags: the resource access flags
> > + *
> > + * Assign a resource to a managed object. This will
> > + * only be called when the object is already locked
> > + * and active. eg, to hotplug a disk into a VM.
> > + * The format of @name varies according to
> > + * the resource @type. A VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK
> > + * will have a fully qualified file path.
> > + *
> > + * If no flags are given, the resource is assumed to be
> > + * used in exclusive, read-write mode. Access can be
> > + * relaxed to readonly, or shared read-write.
> > + *
> > + * Returns 0 on success, or -1 on failure
> > + */
> > +typedef int (*virLockDriverAcquireResource)(virLockManagerPtr man,
> > +                                            unsigned int type,
> > +                                            const char *name,
> > +                                            unsigned int flags);
> 
> This doesn't work with my suggestion above about setting parameters on
> resources between adding them and acquiring them, if that's something we
> want to do.  It also won't allow locking multiple resources together like
> the original acquire does, i.e. acquire all these locks or none.

Daniel