Re: [Qemu-block] [PATCH 40/42] job: Add query-jobs QMP command

[Kevin Wolf]

Am 15.05.2018 um 01:09 hat Max Reitz geschrieben:
> On 2018-05-09 18:26, Kevin Wolf wrote:
> > This adds a minimal query-jobs implementation that shouldn't pose many
> > design questions. It can later be extended to expose more information,
> > and especially job-specific information.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>

> > +##
> > +# @JobInfo:
> > +#
> > +# Information about a job.
> > +#
> > +# @id:                  The job identifier
> > +#
> > +# @type:                The job type
> 
> I'm not really happy with this description that effectively provides no
> information that one cannot already get from the field name, but I
> cannot come up with something better, so I'd rather stop typing now.
> 
> (OK, my issue here is that "job type" can be anything.  That could mean
> e.g. "Is this a block job?".   Maybe an explicit reference to JobType
> might help here, although that is already part of the documentation.  On
> that note, maybe a quote from the documentation might help make my point
> that this description is not very useful:
> 
>        "type: JobType"
>            The job type
> 
> Maybe "The kind of job that is being performed"?)

IMHO, that's just a more verbose way of saying nothing new. The
"problem" is that "type: JobType" is already descriptive enough, there
is no real need for providing any additional information.

Also, I'd like to mention that almost all of the documentation is taken
from BlockJobInfo, so if we decide to change something, we should
probably change it in both places.

> > +# @status:              Current job state/status
> 
> Why the "state/status"?  Forgive me my incompetence, but I don't see a
> real difference here.  But in any case, one ought to be enough, no?
> 
> (OK, full disclosure: My gut tells me "status" is what we now call the
> "progress", and this field should be called "state".  But it's called
> "status" now and it doesn't really make a difference, so it's fine to
> describe it as either.)

I'll defer to John who wrote this description originally. I think we're
just using status/state inconsistently for the same thing (JobStatus,
which represents the states of the job state machine).

> > +#
> > +# @current-progress:    The current progress value
> > +#
> > +# @total-progress:      The maximum progress value
> 
> Hm, not really.  This makes it sound like at any point, this will be the
> maximum even in the future, but that's not true.
> 
> Maybe "estimated progress maximum"?  Or be even more verbose (no, that
> doesn't hurt): "This is an estimation of the value @current-progress
> needs to reach for the job to complete."
> 
> (Actually, I find it important to note that it is an estimation, maybe
> we event want to be really explicit about the fact that this value may
> change all the time, in any direction.)

I'll try to improve the documentation of these fields (both here and in
BlockJobInfo) for v2.

Kevin

signature.asc
Description: PGP signature