Re: [Qemu-devel] [PATCH v6 23/39] block: Prepare for NULL BDS

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 9327 bytes --]

On 13.10.2015 17:37, Kevin Wolf wrote:
> Am 12.10.2015 um 22:00 hat Max Reitz geschrieben:
>> blk_bs() will not necessarily return a non-NULL value any more (unless
>> blk_is_available() is true or it can be assumed to otherwise, e.g.
>> because it is called immediately after a successful blk_new_with_bs() or
>> blk_new_open()).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>> @@ -1584,12 +1594,16 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>>                    "Device '%s' not found", backup->device);
>>          return;
>>      }
>> -    bs = blk_bs(blk);
>>  
>>      /* AioContext is released in .clean() */
>> -    state->aio_context = bdrv_get_aio_context(bs);
>> +    state->aio_context = blk_get_aio_context(blk);
>>      aio_context_acquire(state->aio_context);
>>  
>> +    if (!blk_is_available(blk)) {
>> +        error_setg(errp, "Device '%s' has no medium", backup->device);
>> +        return;
>> +    }
>> +
>>      qmp_drive_backup(backup->device, backup->target,
>>                       backup->has_format, backup->format,
>>                       backup->sync,
>> @@ -1604,7 +1618,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>>          return;
>>      }
>>  
>> -    state->bs = bs;
>> +    state->bs = blk_bs(blk);
>>      state->job = state->bs->job;
>>  }
>>  
>> @@ -1639,8 +1653,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
>>  {
>>      BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
>>      BlockdevBackup *backup;
>> -    BlockDriverState *bs, *target;
>> -    BlockBackend *blk;
>> +    BlockBackend *blk, *target;
>>      Error *local_err = NULL;
>>  
>>      assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
>> @@ -1651,18 +1664,16 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
>>          error_setg(errp, "Device '%s' not found", backup->device);
>>          return;
>>      }
>> -    bs = blk_bs(blk);
>>  
>> -    blk = blk_by_name(backup->target);
>> -    if (!blk) {
>> +    target = blk_by_name(backup->target);
>> +    if (!target) {
>>          error_setg(errp, "Device '%s' not found", backup->target);
>>          return;
>>      }
>> -    target = blk_bs(blk);
>>  
>>      /* AioContext is released in .clean() */
>> -    state->aio_context = bdrv_get_aio_context(bs);
>> -    if (state->aio_context != bdrv_get_aio_context(target)) {
>> +    state->aio_context = blk_get_aio_context(blk);
>> +    if (state->aio_context != blk_get_aio_context(target)) {
>>          state->aio_context = NULL;
>>          error_setg(errp, "Backup between two IO threads is not implemented");
>>          return;
>> @@ -1680,7 +1691,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
>>          return;
>>      }
>>  
>> -    state->bs = bs;
>> +    state->bs = blk_bs(blk);
>>      state->job = state->bs->job;
>>  }
> 
> It's somewhat inconsistent that blockdev_backup_prepare() doesn't do an
> explicit blk_is_available() check whereas drive_backup_prepare() does.
> As far as I can tell, both don't necessarily need their for their own
> code and in both cases the called qmp_*_backup() function checks it
> again.

Probably some kind of a rebase conflict. blockdev-backup was added only
rather recently, and so I made a different decision then than for
drive-backup.

I think I'll drop the check from drive_backup_prepare(). Having it in
qmp_drive_backup() is enough.

> Not really a problem, though, it just looks a bit odd.
> 
>> @@ -1818,10 +1829,10 @@ static void eject_device(BlockBackend *blk, int force, Error **errp)
>>      BlockDriverState *bs = blk_bs(blk);
>>      AioContext *aio_context;
>>  
>> -    aio_context = bdrv_get_aio_context(bs);
>> +    aio_context = blk_get_aio_context(blk);
>>      aio_context_acquire(aio_context);
>>  
>> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
>> +    if (bs && bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
>>          goto out;
>>      }
>>      if (!blk_dev_has_removable_media(blk)) {
>>          error_setg(errp, "Device '%s' is not removable",
>>                     bdrv_get_device_name(bs));
> 
> bs isn't checked to be non-NULL here. You could argue that if it's not
> removable, it shouldn't be NULL, but I think we let drive_del forcibly
> remove the medium even for devices that can't deal with it and start
> producing I/O errors then.

I'd mainly argue that this function is to be removed anyway. :-)

I should just return immediately for NULL bs, as you're suggesting.

> 
>>          goto out;
>>      }
>>
>>      if (blk_dev_is_medium_locked(blk) && !blk_dev_is_tray_open(blk)) {
>>          blk_dev_eject_request(blk, force);
>>          if (!force) {
>>              error_setg(errp, "Device '%s' is locked",
>>                         bdrv_get_device_name(bs));
> 
> And here.
> 
>>              goto out;
>>          }
>>      }
>>  
>> -    bdrv_close(bs);
>> +    if (bs) {
>> +        bdrv_close(bs);
>> +    }
>>  
>>  out:
>>      aio_context_release(aio_context);
> 
> I think the change becomes simpler if you just return immediately for a
> NULL bs instead of evaluation for every place whether checking it could
> make a difference.
> 
>> @@ -1884,10 +1897,12 @@ void qmp_block_passwd(bool has_device, const char *device,
>>  }
>>  
>>  /* Assumes AioContext is held */
>> -static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>> +static void qmp_bdrv_open_encrypted(BlockDriverState **pbs,
>> +                                    const char *filename,
>>                                      int bdrv_flags, const char *format,
>>                                      const char *password, Error **errp)
>>  {
>> +    BlockDriverState *bs;
>>      Error *local_err = NULL;
>>      QDict *options = NULL;
>>      int ret;
>> @@ -1897,11 +1912,12 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>>          qdict_put(options, "driver", qstring_from_str(format));
>>      }
>>  
>> -    ret = bdrv_open(&bs, filename, NULL, options, bdrv_flags, &local_err);
>> +    ret = bdrv_open(pbs, filename, NULL, options, bdrv_flags, &local_err);
>>      if (ret < 0) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>> +    bs = *pbs;
>>  
>>      bdrv_add_key(bs, password, errp);
>>  }
> 
> Yup, we definitely need a local variable bs for this one line! ;-)

*cough cough*

> 
>> @@ -1913,6 +1929,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
>>      BlockDriverState *bs;
>>      AioContext *aio_context;
>>      int bdrv_flags;
>> +    bool new_bs;
>>      Error *err = NULL;
>>  
>>      blk = blk_by_name(device);
>> @@ -1922,8 +1939,9 @@ void qmp_change_blockdev(const char *device, const char *filename,
>>          return;
>>      }
>>      bs = blk_bs(blk);
>> +    new_bs = !bs;
>>  
>> -    aio_context = bdrv_get_aio_context(bs);
>> +    aio_context = blk_get_aio_context(blk);
>>      aio_context_acquire(aio_context);
>>  
>>      eject_device(blk, 0, &err);
>> @@ -1932,10 +1950,18 @@ void qmp_change_blockdev(const char *device, const char *filename,
>>          goto out;
>>      }
>>  
>> -    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>> -    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
>> +    bdrv_flags = blk_is_read_only(blk) ? 0 : BDRV_O_RDWR;
>> +    bdrv_flags |= blk_get_root_state(blk)->open_flags & ~BDRV_O_RDWR;
>>  
>> -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, format, NULL, errp);
>> +    qmp_bdrv_open_encrypted(&bs, filename, bdrv_flags, format, NULL, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
> 
> goto out just to keep working when someone adds another line of code
> before out?

Yes, will do.

>> +    } else if (new_bs) {
>> +        blk_insert_bs(blk, bs);
>> +        /* Has been sent automatically by bdrv_open() if blk_bs(blk) was not
>> +         * NULL */
>> +        blk_dev_change_media_cb(blk, true);
>> +    }
>>  
>>  out:
>>      aio_context_release(aio_context);
> 
>> @@ -2151,16 +2182,19 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>>          return;
>>      }
>>  
>> -    aio_context = bdrv_get_aio_context(bs);
>> +    aio_context = blk_get_aio_context(blk);
>>      aio_context_acquire(aio_context);
>>  
>> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
>> +    bs = blk_bs(blk);
>> +    if (bs && bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
>>          error_report_err(local_err);
>>          aio_context_release(aio_context);
>>          return;
>>      }
>>  
>> -    bdrv_close(bs);
>> +    if (bs) {
>> +        bdrv_close(bs);
>> +    }
> 
> Why not a single if (bs) block?

Because that would mean more lines modified. Every line git blame
associates with me is a line Markus might ask me something about.

I'll put it into a single block.

As always, thanks for reviewing!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]