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 <kw...@redhat.com>
>>> ---
>>> 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
