Re: [Qemu-block] [RFC PATCH 16/41] block: Add error parameter to blk_insert_bs()

[Max Reitz]

On 20.02.2017 12:22, Kevin Wolf wrote:
> Am 20.02.2017 um 12:04 hat Max Reitz geschrieben:
>> On 13.02.2017 18:22, Kevin Wolf wrote:
>>> Mow that blk_insert_bs() requests the BlockBackend permissions for the
>>> node it attaches to, it can fail. Instead of aborting, pass the errors
>>> to the callers.
>>
>> So it does qualify as a FIXME, but just for a single patch. Good. :-)
>>
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>  block/backup.c                   |  5 ++++-
>>>  block/block-backend.c            | 12 ++++++++----
>>>  block/commit.c                   | 38 
>>> ++++++++++++++++++++++++++++++--------
>>>  block/mirror.c                   | 15 ++++++++++++---
>>>  blockdev.c                       |  6 +++++-
>>>  blockjob.c                       |  7 ++++++-
>>>  hmp.c                            |  6 +++++-
>>>  hw/core/qdev-properties-system.c |  7 ++++++-
>>>  include/sysemu/block-backend.h   |  2 +-
>>>  migration/block.c                |  2 +-
>>>  nbd/server.c                     |  6 +++++-
>>>  tests/test-blockjob.c            |  2 +-
>>>  12 files changed, 84 insertions(+), 24 deletions(-)
>>
>> [...]
>>
>>> diff --git a/migration/block.c b/migration/block.c
>>> index 6b7ffd4..d259936 100644
>>> --- a/migration/block.c
>>> +++ b/migration/block.c
>>> @@ -446,7 +446,7 @@ static void init_blk_migration(QEMUFile *f)
>>>          BlockDriverState *bs = bmds_bs[i].bs;
>>>  
>>>          if (bmds) {
>>> -            blk_insert_bs(bmds->blk, bs);
>>> +            blk_insert_bs(bmds->blk, bs, &error_abort);
>>
>> I don't think it's obvious why this is correct. I assume it is because
>> this was a legal configuration on the source, so it must be a legal
>> configuration on the destination.
>>
>> But I'd certainly appreciate a comment to make that explicitly clear,
>> especially considering that it isn't obvious that blk_insert_bs() can
>> fail only because of op blockers and thus will always work if the
>> configuration is known to be legal.
> 
> Actually, it's just because the requested permissions for bmds->blk are
> still 0, BLK_PERM_ALL here. Once the FIXME is addressed (patch 37) and
> the real permissions are requested, we get some error handling here.

OK, good, thanks.

Max

signature.asc
Description: OpenPGP digital signature