Re: [Qemu-devel] [PATCH 2/3] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 11.03.2015 um 17:38 hat Alberto Garcia geschrieben:
> On Thu, Mar 05, 2015 at 03:09:58PM +0100, Kevin Wolf wrote:
> 
> > >  { 'command': 'block-stream',
> > > -  'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
> > > -            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
> > > +  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
> > > +            '*backing-file': 'str', '*speed': 'int',
> > > +            '*on-error': 'BlockdevOnError' } }
> > 
> > There is no point in specifying some root node as 'device' that
> > isn't actually involved in the operation; worse, it isn't even
> > possible in the general case because 'top' could have multiple
> > users/parents.
> > 
> > A better interface would probably be to allow node names for
> > 'device' and leave everything else as it is.
> 
> Ok, I changed the code and it does make the implementation simpler.
> 
> One issue that I'm finding is that when we move the block-stream
> job to an intermediate node, where the device name is empty, we get
> messages like "Device '' is busy".
> 
> I can use node names instead, but they are also not guaranteed to
> exist. I heard there was a plan to auto-generate names, and searching
> the archives I found this patch by Jeff Cody:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg04057.html
> 
> But it seems that it was never merged?
> 
> If we are going to have a scenario where a parameter can mean either a
> device or a node name, we need a clear way to identify that node.

Yes, autogenerated node names were not merged yet. And if they were,
they wouldn't make for very good error messages either.

My first thought was "then make it 'Source/Target device is busy'
without mentioning any name". In the context of any given command, it
would still be clear which BDS is meant. In fact, I have argued before
that mentioning the device name in an error to a command that refers to
a specific device is redundant and should be avoided.

The problem here is that it's not stream_start() that generates the
error, but block_job_create(), which doesn't know which role it's bs
argument has for the block job. So it can't decide whether to say
"source device", "target device" or something completely different.

On the other hand, having an owner BDS for a block job is considered a
mistake meanwhile because there is no clear rule which BDS to pick when
the job involves more than one. In fact, without tying a job to a BDS,
it could be just a background job instead of specifically a block job.
I'm not saying that this conversion should be done now, but just to give
you some background about the direction we're generally taking.

So in the light of this, it might be reasonable to move the bs->job
check with the error check to the callers.

Another, less invasive, option would be to replace the error_set() call
in block_job_create() by error_copy(bs->job->blocker). We're not really
op blockers code here, so might be somewhat ugly, but I think eventually
the check is going to be fully replaced by op blockers anyway, so using
the same message now could make sense.

Jeff, as you are working on op blockers, do you have an opinion on this?

Kevin