From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51749) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmNdN-0003Va-1g for qemu-devel@nongnu.org; Wed, 14 Oct 2015 11:06:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmNdL-0001wU-HN for qemu-devel@nongnu.org; Wed, 14 Oct 2015 11:06:28 -0400 References: <1444680042-13207-1-git-send-email-mreitz@redhat.com> <1444680042-13207-24-git-send-email-mreitz@redhat.com> <20151013153727.GK4906@noname.str.redhat.com> From: Max Reitz Message-ID: <561E6F65.7090705@redhat.com> Date: Wed, 14 Oct 2015 17:06:13 +0200 MIME-Version: 1.0 In-Reply-To: <20151013153727.GK4906@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ufLLvo9tnB4cW7eXKU97xnmVQlCU1Uoqf" Subject: Re: [Qemu-devel] [PATCH v6 23/39] block: Prepare for NULL BDS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Alberto Garcia , qemu-block@nongnu.org, John Snow , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --ufLLvo9tnB4cW7eXKU97xnmVQlCU1Uoqf Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 >=20 >> @@ -1584,12 +1594,16 @@ static void drive_backup_prepare(BlkTransactio= nState *common, Error **errp) >> "Device '%s' not found", backup->device); >> return; >> } >> - bs =3D blk_bs(blk); >> =20 >> /* AioContext is released in .clean() */ >> - state->aio_context =3D bdrv_get_aio_context(bs); >> + state->aio_context =3D blk_get_aio_context(blk); >> aio_context_acquire(state->aio_context); >> =20 >> + 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(BlkTransactionS= tate *common, Error **errp) >> return; >> } >> =20 >> - state->bs =3D bs; >> + state->bs =3D blk_bs(blk); >> state->job =3D state->bs->job; >> } >> =20 >> @@ -1639,8 +1653,7 @@ static void blockdev_backup_prepare(BlkTransacti= onState *common, Error **errp) >> { >> BlockdevBackupState *state =3D DO_UPCAST(BlockdevBackupState, com= mon, common); >> BlockdevBackup *backup; >> - BlockDriverState *bs, *target; >> - BlockBackend *blk; >> + BlockBackend *blk, *target; >> Error *local_err =3D NULL; >> =20 >> assert(common->action->kind =3D=3D TRANSACTION_ACTION_KIND_BLOCKD= EV_BACKUP); >> @@ -1651,18 +1664,16 @@ static void blockdev_backup_prepare(BlkTransac= tionState *common, Error **errp) >> error_setg(errp, "Device '%s' not found", backup->device); >> return; >> } >> - bs =3D blk_bs(blk); >> =20 >> - blk =3D blk_by_name(backup->target); >> - if (!blk) { >> + target =3D blk_by_name(backup->target); >> + if (!target) { >> error_setg(errp, "Device '%s' not found", backup->target); >> return; >> } >> - target =3D blk_bs(blk); >> =20 >> /* AioContext is released in .clean() */ >> - state->aio_context =3D bdrv_get_aio_context(bs); >> - if (state->aio_context !=3D bdrv_get_aio_context(target)) { >> + state->aio_context =3D blk_get_aio_context(blk); >> + if (state->aio_context !=3D blk_get_aio_context(target)) { >> state->aio_context =3D NULL; >> error_setg(errp, "Backup between two IO threads is not implem= ented"); >> return; >> @@ -1680,7 +1691,7 @@ static void blockdev_backup_prepare(BlkTransacti= onState *common, Error **errp) >> return; >> } >> =20 >> - state->bs =3D bs; >> + state->bs =3D blk_bs(blk); >> state->job =3D state->bs->job; >> } >=20 > 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. >=20 >> @@ -1818,10 +1829,10 @@ static void eject_device(BlockBackend *blk, in= t force, Error **errp) >> BlockDriverState *bs =3D blk_bs(blk); >> AioContext *aio_context; >> =20 >> - aio_context =3D bdrv_get_aio_context(bs); >> + aio_context =3D blk_get_aio_context(blk); >> aio_context_acquire(aio_context); >> =20 >> - 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)); >=20 > 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. >=20 >> 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)); >=20 > And here. >=20 >> goto out; >> } >> } >> =20 >> - bdrv_close(bs); >> + if (bs) { >> + bdrv_close(bs); >> + } >> =20 >> out: >> aio_context_release(aio_context); >=20 > 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. >=20 >> @@ -1884,10 +1897,12 @@ void qmp_block_passwd(bool has_device, const c= har *device, >> } >> =20 >> /* 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 *forma= t, >> const char *password, Error **err= p) >> { >> + BlockDriverState *bs; >> Error *local_err =3D NULL; >> QDict *options =3D NULL; >> int ret; >> @@ -1897,11 +1912,12 @@ static void qmp_bdrv_open_encrypted(BlockDrive= rState *bs, const char *filename, >> qdict_put(options, "driver", qstring_from_str(format)); >> } >> =20 >> - ret =3D bdrv_open(&bs, filename, NULL, options, bdrv_flags, &loca= l_err); >> + ret =3D bdrv_open(pbs, filename, NULL, options, bdrv_flags, &loca= l_err); >> if (ret < 0) { >> error_propagate(errp, local_err); >> return; >> } >> + bs =3D *pbs; >> =20 >> bdrv_add_key(bs, password, errp); >> } >=20 > Yup, we definitely need a local variable bs for this one line! ;-) *cough cough* >=20 >> @@ -1913,6 +1929,7 @@ void qmp_change_blockdev(const char *device, con= st char *filename, >> BlockDriverState *bs; >> AioContext *aio_context; >> int bdrv_flags; >> + bool new_bs; >> Error *err =3D NULL; >> =20 >> blk =3D blk_by_name(device); >> @@ -1922,8 +1939,9 @@ void qmp_change_blockdev(const char *device, con= st char *filename, >> return; >> } >> bs =3D blk_bs(blk); >> + new_bs =3D !bs; >> =20 >> - aio_context =3D bdrv_get_aio_context(bs); >> + aio_context =3D blk_get_aio_context(blk); >> aio_context_acquire(aio_context); >> =20 >> eject_device(blk, 0, &err); >> @@ -1932,10 +1950,18 @@ void qmp_change_blockdev(const char *device, c= onst char *filename, >> goto out; >> } >> =20 >> - bdrv_flags =3D bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR; >> - bdrv_flags |=3D bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0; >> + bdrv_flags =3D blk_is_read_only(blk) ? 0 : BDRV_O_RDWR; >> + bdrv_flags |=3D blk_get_root_state(blk)->open_flags & ~BDRV_O_RDW= R; >> =20 >> - qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, format, NULL, e= rrp); >> + qmp_bdrv_open_encrypted(&bs, filename, bdrv_flags, format, NULL, = &err); >> + if (err) { >> + error_propagate(errp, err); >=20 > 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); >> + } >> =20 >> out: >> aio_context_release(aio_context); >=20 >> @@ -2151,16 +2182,19 @@ void hmp_drive_del(Monitor *mon, const QDict *= qdict) >> return; >> } >> =20 >> - aio_context =3D bdrv_get_aio_context(bs); >> + aio_context =3D blk_get_aio_context(blk); >> aio_context_acquire(aio_context); >> =20 >> - if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) = { >> + bs =3D 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; >> } >> =20 >> - bdrv_close(bs); >> + if (bs) { >> + bdrv_close(bs); >> + } >=20 > 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 --ufLLvo9tnB4cW7eXKU97xnmVQlCU1Uoqf Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWHm9lAAoJEDuxQgLoOKyt8MwIAIQSUEr5CxSb2Inlr6huKawc MuT1rB2wVWRjxgMOmr17bQEdsTnZRw4oPjgaAEZG/HvxXC1EytR7u8Uca9sfPhvU JJm3CbvINa7e2Q0AVROqGXUBigbsp9ujL/SHQS65r6S8Z7/FHBw+Uk74Q1Nkf48G 6UKPFTa1X49qYxdyPEkEDrVu/idRF1eibJj6V4MR+nhOeyFbeZUJ+CZ1mCKOs+/d BpjBF3iSw7nLJPZWZvFLvauEpq8N0mp9yIklrekerFka+TENBb839Z0+GQXzcR0p GdH5TwIuICGZ7ALMZppfppPx6S7SuWyYSJtmBHpF5xUBwm6ag9VoQMJVhkMEj5A= =2jT/ -----END PGP SIGNATURE----- --ufLLvo9tnB4cW7eXKU97xnmVQlCU1Uoqf--