* [Qemu-devel] [PATCH] blockdev: fix missed target unref for drive-backup
@ 2019-05-10 21:52 John Snow
2019-05-13 11:13 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: John Snow @ 2019-05-10 21:52 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Kevin Wolf, aihua liang, John Snow, Markus Armbruster, Max Reitz
If the bitmap can't be used for whatever reason, we skip putting down
the reference. Fix that.
In practice, this means that if you attempt to gracefully exit QEMU
after a backup command being rejected, bdrv_close_all will fail and
tell you some unpleasant things via assert().
Reported-by: aihua liang <aliang@redhat.com>
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1703916
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockdev.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 79fbac8450..278ecdd122 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3525,8 +3525,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
if (set_backing_hd) {
bdrv_set_backing_hd(target_bs, source, &local_err);
if (local_err) {
- bdrv_unref(target_bs);
- goto out;
+ goto unref;
}
}
@@ -3534,11 +3533,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
if (!bmap) {
error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
- bdrv_unref(target_bs);
- goto out;
+ goto unref;
}
if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
- goto out;
+ goto unref;
}
}
if (!backup->auto_finalize) {
@@ -3552,12 +3550,12 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
backup->sync, bmap, backup->compress,
backup->on_source_error, backup->on_target_error,
job_flags, NULL, NULL, txn, &local_err);
- bdrv_unref(target_bs);
if (local_err != NULL) {
error_propagate(errp, local_err);
- goto out;
}
+unref:
+ bdrv_unref(target_bs);
out:
aio_context_release(aio_context);
return job;
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] blockdev: fix missed target unref for drive-backup
2019-05-10 21:52 [Qemu-devel] [PATCH] blockdev: fix missed target unref for drive-backup John Snow
@ 2019-05-13 11:13 ` Kevin Wolf
2019-05-13 15:01 ` John Snow
0 siblings, 1 reply; 3+ messages in thread
From: Kevin Wolf @ 2019-05-13 11:13 UTC (permalink / raw)
To: John Snow
Cc: Max Reitz, aihua liang, qemu-devel, qemu-block, Markus Armbruster
Am 10.05.2019 um 23:52 hat John Snow geschrieben:
> If the bitmap can't be used for whatever reason, we skip putting down
> the reference. Fix that.
>
> In practice, this means that if you attempt to gracefully exit QEMU
> after a backup command being rejected, bdrv_close_all will fail and
> tell you some unpleasant things via assert().
>
> Reported-by: aihua liang <aliang@redhat.com>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1703916
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> blockdev.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 79fbac8450..278ecdd122 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3525,8 +3525,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
> if (set_backing_hd) {
> bdrv_set_backing_hd(target_bs, source, &local_err);
> if (local_err) {
> - bdrv_unref(target_bs);
> - goto out;
> + goto unref;
> }
> }
>
> @@ -3534,11 +3533,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
> bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
> if (!bmap) {
> error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
> - bdrv_unref(target_bs);
> - goto out;
> + goto unref;
> }
> if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
> - goto out;
> + goto unref;
> }
> }
> if (!backup->auto_finalize) {
> @@ -3552,12 +3550,12 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
> backup->sync, bmap, backup->compress,
> backup->on_source_error, backup->on_target_error,
> job_flags, NULL, NULL, txn, &local_err);
> - bdrv_unref(target_bs);
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> - goto out;
Please leave a 'goto unref' here so we can later add more code without
modifying unrelated error paths (or, more likely, forgetting to modify
them).
> }
>
> +unref:
> + bdrv_unref(target_bs);
> out:
> aio_context_release(aio_context);
> return job;
With this fixed:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] blockdev: fix missed target unref for drive-backup
2019-05-13 11:13 ` Kevin Wolf
@ 2019-05-13 15:01 ` John Snow
0 siblings, 0 replies; 3+ messages in thread
From: John Snow @ 2019-05-13 15:01 UTC (permalink / raw)
To: Kevin Wolf
Cc: Max Reitz, aihua liang, qemu-devel, qemu-block, Markus Armbruster
On 5/13/19 7:13 AM, Kevin Wolf wrote:
> Am 10.05.2019 um 23:52 hat John Snow geschrieben:
>> If the bitmap can't be used for whatever reason, we skip putting down
>> the reference. Fix that.
>>
>> In practice, this means that if you attempt to gracefully exit QEMU
>> after a backup command being rejected, bdrv_close_all will fail and
>> tell you some unpleasant things via assert().
>>
>> Reported-by: aihua liang <aliang@redhat.com>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1703916
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> blockdev.c | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 79fbac8450..278ecdd122 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3525,8 +3525,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>> if (set_backing_hd) {
>> bdrv_set_backing_hd(target_bs, source, &local_err);
>> if (local_err) {
>> - bdrv_unref(target_bs);
>> - goto out;
>> + goto unref;
>> }
>> }
>>
>> @@ -3534,11 +3533,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>> bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
>> if (!bmap) {
>> error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
>> - bdrv_unref(target_bs);
>> - goto out;
>> + goto unref;
>> }
>> if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
>> - goto out;
>> + goto unref;
>> }
>> }
>> if (!backup->auto_finalize) {
>> @@ -3552,12 +3550,12 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>> backup->sync, bmap, backup->compress,
>> backup->on_source_error, backup->on_target_error,
>> job_flags, NULL, NULL, txn, &local_err);
>> - bdrv_unref(target_bs);
>> if (local_err != NULL) {
>> error_propagate(errp, local_err);
>> - goto out;
>
> Please leave a 'goto unref' here so we can later add more code without
> modifying unrelated error paths (or, more likely, forgetting to modify
> them).
>
Oh, that makes sense.
>> }
>>
>> +unref:
>> + bdrv_unref(target_bs);
>> out:
>> aio_context_release(aio_context);
>> return job;
>
> With this fixed:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-13 15:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-10 21:52 [Qemu-devel] [PATCH] blockdev: fix missed target unref for drive-backup John Snow
2019-05-13 11:13 ` Kevin Wolf
2019-05-13 15:01 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.