[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 3/9] blockjob: Don't set iostatus of target
Date: Fri, 6 May 2016 15:40:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0

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.


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]