Re: [Qemu-devel] [PATCH v2 3/9] blockjob: Don't set iostatus of target

[Kevin Wolf]

Am 06.05.2016 um 15:40 hat Max Reitz geschrieben:
> On 06.05.2016 15:31, Kevin Wolf wrote:
> > Am 06.05.2016 um 14:32 hat Max Reitz geschrieben:
> >> On 06.05.2016 14:01, Max Reitz wrote:
> >>> On 27.04.2016 15:20, Kevin Wolf wrote:
> >>>> When block job errors were introduced, we assigned the iostatus of the
> >>>> target BDS "just in case". The field has never been accessible for the
> >>>> user because the target isn't listed in query-block.
> >>>>
> >>>> Before we can allow the user to have a second BlockBackend on the
> >>>> target, we need to clean this up. If anything, we would want to set the
> >>>> iostatus for the internal BB of the job (which we can always do later),
> >>>> but certainly not for a separate BB which the job doesn't even use.
> >>>>
> >>>> As a nice side effect, this gets us rid of another bs->blk use.
> >>>>
> >>>> Signed-off-by: Kevin Wolf <address@hidden>
> >>>> ---
> >>>>  block/backup.c           | 8 ++++----
> >>>>  block/mirror.c           | 8 ++++----
> >>>>  block/stream.c           | 3 +--
> >>>>  blockjob.c               | 6 +-----
> >>>>  include/block/blockjob.h | 4 +---
> >>>>  5 files changed, 11 insertions(+), 18 deletions(-)
> >>>
> >>> Reviewed-by: Max Reitz <address@hidden>
> >>
> >> Maybe I need to take that back. Using e.g. blockdev-backup, you can
> >> indeed see the target in query-block.
> >>
> >> e.g.:
> >>
> >> (echo "{'execute':'qmp_capabilities'}
> >>        {'execute':'blockdev-backup',
> >>         'arguments':{'device':'src','target':'dst',
> >>         'sync':'full','on-target-error':'enospc'}}";
> >>  sleep 1;
> >>  echo "{'execute':'query-block'}") | \
> >>    x86_64-softmmu/qemu-system-x86_64 \
> >>        -drive if=none,id=src,file=test.qcow2 \
> >>        -drive if=none,id=dst,file=mnt/out.qcow2 \
> >>        -qmp-pretty stdio -nodefaults
> >>
> >> Where mnt is a file system and test.qcow2 is large enough such that an
> >> ENOSPC will occur.
> > 
> > Hm... Let's pretend we didn't notice. In fact, I don't think this
> > behaviour is documented and it also doesn't make a lot of sense.
> > 
> > I would be surprised if libvirt made use of it, as there is still the
> > job iostatus which works in drive-* cases, too.
> > 
> > (Eric?)
> > 
> >> Before this patch, you can see "io-status": "nospace" in query-block for
> >> dst. After it, it says "io-status": "ok", and after the next it doesn't
> >> say anything about the status at all anymore.
> > 
> > Simply letting the field disappear sounds like a nice solution.
> 
> I'm not sure I completely agree with that because the iostatus appears
> to be the only way of obtaining the information whether the block job
> stopped because of an ENOSPC or because of something different (the
> BLOCK_JOB_ERROR doesn't tell you).
> 
> I don't know whether anyone actually needs that information, though, and
> if you think dropping this is fine, then I guess it's fine with me, too,
> and my R-b stands.

But you don't have this information with the drive-* commands either.

I would be okay with reintroducing this information, but in the right
place. Modifying the guest device iostatus for block jobs certainly
isn't right.

Anyway, we need Eric's input to confirm that libvirt doesn't depend on
this.

Kevin

pgpyXDwl5hNFv.pgp
Description: PGP signature