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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
