On 06.05.2016 16:12, Kevin Wolf wrote: > 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 <[email protected]> >>>>>> --- >>>>>> 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 <[email protected]> >>>> >>>> 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.
As discussed on IRC, that sounds right to me. Thus, have an explicit Reviewed-by: Max Reitz <[email protected]> > Anyway, we need Eric's input to confirm that libvirt doesn't depend on > this. > > Kevin >
signature.asc
Description: OpenPGP digital signature
