[libvirt] [RFC] New domain job control and stat APIs

[Pavel Hrdina]

On Wed, Jul 24, 2019 at 03:43:14PM +0100, Daniel P. Berrangé wrote:

[...]

> > +typedef struct _virDomainJob virDomainJob;
> > +typedef virDomainJob *virDomainJobPtr;
> > +struct _virDomainJob {
> > +    char *name;
> > +    virDomainJobType type;
> > +    virDomainJobState state;
> > +
> > +    /* possibly overkill? - currently empty*/
> > +    virTypedParameterPtr data;
> > +    size_t ndata;
> > +};
> 
> I'd have a preference for keeping this as an opaque
> struct not exposed in the public header, and making
> it a first class object. So instead have accessors
> 
>   char * virDomainJobGetName(virDomainJobPtr job);
>   virDomainJobType virDOmainJobGetType(virDomainJobPtr job);
>   virDomainJobState virDOmainJobGetState(virDomainJobPtr job);
> 
> which fetch from the local struct. The local struct would
> also need to contain a reference to the virDomainPtrpass over
> 
> This avoids the query you have about virTypedParameterPtr
> inclusion, as we can ignore it now, and if we get a compelling
> need, we then just add a remotable method:
> 
>    virDomainJobGetStats(virDomainJobPtr job,
>                         virTypedParameterPtr **params,
> 			unsigned int *nparams);

Agreed to make it first class object.  Shouldn't we rename it to
virJob* as well or do we want to have it tied to domains only?

> > +void virDomainJobFree(virDomainJobPtr job);
> > +
> > +int virDomainJobList(virDomainPtr domain,
> > +                     virDomainJobPtr **jobs,
> > +                     unsigned int flags);
> > +
> > +int virDomainJobGetXMLDesc(virDomainPtr domain,
> > +                           const char *jobname,
> > +                           unsigned int flags);
> 
> This would become
> 
>  int virDomainJobGetXMLDesc(virDomainJobPtr job,
>                             unsigned int flags);
> 
> 
> > +
> > +typedef enum {
> > +    VIR_DOMAIN_JOB_CONTROL_OPERATION_NONE = 0,
> > +    VIR_DOMAIN_JOB_CONTROL_OPERATION_ABORT = 1,
> > +    VIR_DOMAIN_JOB_CONTROL_OPERATION_FINALIZE = 2,
> > +    VIR_DOMAIN_JOB_CONTROL_OPERATION_PAUSE = 3,
> > +    VIR_DOMAIN_JOB_CONTROL_OPERATION_RESUME = 4,
> > +    VIR_DOMAIN_JOB_CONTROL_OPERATION_DISMISS = 5,
> > +
> > +# ifdef VIR_ENUM_SENTINELS
> > +    VIR_DOMAIN_JOB_CONTROL_OPERATION_LAST
> > +# endif
> > +} virDomainJobControlOperation;
> > +
> > +int virDomainJobControl(virDomainPtr domain,
> > +                        const char *jobname,
> > +                        virDomainJobControlOperation op,
> > +                        unsigned int flags);
> 
> And again change to
> 
>  int virDomainJobControl(virDomainJobPtr job,
>                          virDomainJobControlOperation op,
>                          unsigned int flags);
> 
> 
> I think I'd also have a preference to identify a job based on a UUID too,
> as that gives stronger uniqueness. eg when debugging there's no risk of
> confusing two jobs which are against different domains but happened to get
> the same name. 

Agreed, we need to have a unique ID and we should definitely avoid any
magic with ID=0 to pick the "current" job as that would only lead to lot
of issues.

Overall looks good.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190725/4c0ffe72/attachment-0001.sig>