* [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return
@ 2016-06-30 8:01 Changlong Xie
2016-06-30 8:25 ` Fam Zheng
0 siblings, 1 reply; 6+ messages in thread
From: Changlong Xie @ 2016-06-30 8:01 UTC (permalink / raw)
To: qemu devel
Cc: Stefan Hajnoczi, Fam Zheng, Max Reitz, Kevin Wolf, Jeff Cody,
Changlong Xie
Otherwise, we could never trigger assert(!bitmap->successor)
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
block/dirty-bitmap.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4902ca5..e9df5ac 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -131,7 +131,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
if (bdrv_dirty_bitmap_frozen(bitmap)) {
error_setg(errp, "Cannot create a successor for a bitmap that is "
"currently frozen");
- return -1;
}
assert(!bitmap->successor);
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return
2016-06-30 8:01 [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return Changlong Xie
@ 2016-06-30 8:25 ` Fam Zheng
2016-06-30 8:45 ` Changlong Xie
0 siblings, 1 reply; 6+ messages in thread
From: Fam Zheng @ 2016-06-30 8:25 UTC (permalink / raw)
To: Changlong Xie
Cc: qemu devel, Kevin Wolf, Jeff Cody, Max Reitz, Stefan Hajnoczi
On Thu, 06/30 16:01, Changlong Xie wrote:
> Otherwise, we could never trigger assert(!bitmap->successor)
>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
> block/dirty-bitmap.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4902ca5..e9df5ac 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -131,7 +131,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> if (bdrv_dirty_bitmap_frozen(bitmap)) {
> error_setg(errp, "Cannot create a successor for a bitmap that is "
> "currently frozen");
> - return -1;
> }
> assert(!bitmap->successor);
This is wrong. Then we will always trigger assert for a frozen bitmap.
Fam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return
2016-06-30 8:25 ` Fam Zheng
@ 2016-06-30 8:45 ` Changlong Xie
2016-06-30 14:00 ` Jeff Cody
0 siblings, 1 reply; 6+ messages in thread
From: Changlong Xie @ 2016-06-30 8:45 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu devel, Kevin Wolf, Jeff Cody, Max Reitz, Stefan Hajnoczi
On 06/30/2016 04:25 PM, Fam Zheng wrote:
> On Thu, 06/30 16:01, Changlong Xie wrote:
>> Otherwise, we could never trigger assert(!bitmap->successor)
>>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> ---
>> block/dirty-bitmap.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 4902ca5..e9df5ac 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -131,7 +131,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>> if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> error_setg(errp, "Cannot create a successor for a bitmap that is "
>> "currently frozen");
>> - return -1;
>> }
>> assert(!bitmap->successor);
>
> This is wrong. Then we will always trigger assert for a frozen bitmap.
>
IMO, when it's a frozen bitmap, we will always return -1. So
"assert(!bitmap->successor)" is useless here, am i right?
> Fam
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return
2016-06-30 8:45 ` Changlong Xie
@ 2016-06-30 14:00 ` Jeff Cody
2016-06-30 18:18 ` John Snow
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Cody @ 2016-06-30 14:00 UTC (permalink / raw)
To: Changlong Xie
Cc: Fam Zheng, qemu devel, Kevin Wolf, Max Reitz, Stefan Hajnoczi
On Thu, Jun 30, 2016 at 04:45:52PM +0800, Changlong Xie wrote:
> On 06/30/2016 04:25 PM, Fam Zheng wrote:
> >On Thu, 06/30 16:01, Changlong Xie wrote:
> >>Otherwise, we could never trigger assert(!bitmap->successor)
> >>
> >>Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> >>---
> >> block/dirty-bitmap.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >>diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> >>index 4902ca5..e9df5ac 100644
> >>--- a/block/dirty-bitmap.c
> >>+++ b/block/dirty-bitmap.c
> >>@@ -131,7 +131,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> >> if (bdrv_dirty_bitmap_frozen(bitmap)) {
> >> error_setg(errp, "Cannot create a successor for a bitmap that is "
> >> "currently frozen");
> >>- return -1;
> >> }
> >> assert(!bitmap->successor);
> >
> >This is wrong. Then we will always trigger assert for a frozen bitmap.
> >
>
> IMO, when it's a frozen bitmap, we will always return -1. So
> "assert(!bitmap->successor)" is useless here, am i right?
>
I don't see a path where the assert could trigger, so I would agree that the
assert itself, while harmless, is not necessary (although it could be argued
it is in place in case the code above it changes in a way that does not
check bitmap->successor).
That doesn't mean we want to try and trigger an assert, however! :) The
error return is the proper error handling -- we don't expect that asserts
should ever be encountered QEMU, if one happens that is a sign of a bug.
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return
2016-06-30 14:00 ` Jeff Cody
@ 2016-06-30 18:18 ` John Snow
2016-07-01 1:20 ` Changlong Xie
0 siblings, 1 reply; 6+ messages in thread
From: John Snow @ 2016-06-30 18:18 UTC (permalink / raw)
To: Jeff Cody, Changlong Xie
Cc: Kevin Wolf, Fam Zheng, qemu devel, Stefan Hajnoczi, Max Reitz
On 06/30/2016 10:00 AM, Jeff Cody wrote:
> On Thu, Jun 30, 2016 at 04:45:52PM +0800, Changlong Xie wrote:
>> On 06/30/2016 04:25 PM, Fam Zheng wrote:
>>> On Thu, 06/30 16:01, Changlong Xie wrote:
>>>> Otherwise, we could never trigger assert(!bitmap->successor)
>>>>
>>>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>>>> ---
>>>> block/dirty-bitmap.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>> index 4902ca5..e9df5ac 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -131,7 +131,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>>> if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>>> error_setg(errp, "Cannot create a successor for a bitmap that is "
>>>> "currently frozen");
>>>> - return -1;
>>>> }
>>>> assert(!bitmap->successor);
>>>
>>> This is wrong. Then we will always trigger assert for a frozen bitmap.
>>>
>>
>> IMO, when it's a frozen bitmap, we will always return -1. So
>> "assert(!bitmap->successor)" is useless here, am i right?
>>
>
> I don't see a path where the assert could trigger, so I would agree that the
> assert itself, while harmless, is not necessary (although it could be argued
> it is in place in case the code above it changes in a way that does not
> check bitmap->successor).
>
> That doesn't mean we want to try and trigger an assert, however! :) The
> error return is the proper error handling -- we don't expect that asserts
> should ever be encountered QEMU, if one happens that is a sign of a bug.
>
> Jeff
>
The assert was indeed added to ensure that if the valid states of the
bitmap later expanded or changed, that the status checkers (e.g.
bdrv_dirty_bitmap_frozen()) were changed to match.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return
2016-06-30 18:18 ` John Snow
@ 2016-07-01 1:20 ` Changlong Xie
0 siblings, 0 replies; 6+ messages in thread
From: Changlong Xie @ 2016-07-01 1:20 UTC (permalink / raw)
To: John Snow, Jeff Cody
Cc: Kevin Wolf, Fam Zheng, qemu devel, Stefan Hajnoczi, Max Reitz
On 07/01/2016 02:18 AM, John Snow wrote:
>
>
> On 06/30/2016 10:00 AM, Jeff Cody wrote:
>> On Thu, Jun 30, 2016 at 04:45:52PM +0800, Changlong Xie wrote:
>>> On 06/30/2016 04:25 PM, Fam Zheng wrote:
>>>> On Thu, 06/30 16:01, Changlong Xie wrote:
>>>>> Otherwise, we could never trigger assert(!bitmap->successor)
>>>>>
>>>>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>>>>> ---
>>>>> block/dirty-bitmap.c | 1 -
>>>>> 1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>>> index 4902ca5..e9df5ac 100644
>>>>> --- a/block/dirty-bitmap.c
>>>>> +++ b/block/dirty-bitmap.c
>>>>> @@ -131,7 +131,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>>>> if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>>>> error_setg(errp, "Cannot create a successor for a bitmap that is "
>>>>> "currently frozen");
>>>>> - return -1;
>>>>> }
>>>>> assert(!bitmap->successor);
>>>>
>>>> This is wrong. Then we will always trigger assert for a frozen bitmap.
>>>>
>>>
>>> IMO, when it's a frozen bitmap, we will always return -1. So
>>> "assert(!bitmap->successor)" is useless here, am i right?
>>>
>>
>> I don't see a path where the assert could trigger, so I would agree that the
>> assert itself, while harmless, is not necessary (although it could be argued
>> it is in place in case the code above it changes in a way that does not
>> check bitmap->successor).
Agree
>>
>> That doesn't mean we want to try and trigger an assert, however! :) The
>> error return is the proper error handling -- we don't expect that asserts
>> should ever be encountered QEMU, if one happens that is a sign of a bug.
>>
Got it
>> Jeff
>>
>
> The assert was indeed added to ensure that if the valid states of the
> bitmap later expanded or changed, that the status checkers (e.g.
> bdrv_dirty_bitmap_frozen()) were changed to match.
>
Thanks for all explanations. Although my brain always forces to think
it's a redundant execution path, but since it's harmless, let's keep it.
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-01 1:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-30 8:01 [Qemu-devel] [PATCH] dirty-bitmap: remove unnecessary return Changlong Xie
2016-06-30 8:25 ` Fam Zheng
2016-06-30 8:45 ` Changlong Xie
2016-06-30 14:00 ` Jeff Cody
2016-06-30 18:18 ` John Snow
2016-07-01 1:20 ` Changlong Xie
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.