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