Re: [Qemu-devel] [PATCH v4 1/3] block: include base when checking image chain for block allocation
On Tue 09 Apr 2019 04:43:12 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>> -while (intermediate && intermediate != base) {
>>> +while (include_base || intermediate != base) {
>>> int64_t pnum_inter;
>>> int64_t size_inter;
>>>
>>> @@ -2360,6 +2364,10 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>> n = pnum_inter;
>>> }
>>>
>>> +if (intermediate == base) {
>>> +break;
>>> +}
>>> +
>>> intermediate = backing_bs(intermediate);
>>
>> I find that the new condition + the break make things a bit less
>> readable. I think it would be simpler with something like this:
>>
>> BlockDriverState *stop_at = include_base ? backing_bs(base) : base;
>>
>> while (intermediate != stop_at) {
>>...
>> }
>>
>
> But in this way you return back dependence on base, which we don't
> freeze and which may disappear on some iteration. We should not touch
> backing_bs(base) in any way.
Ok, I see.
Reviewed-by: Alberto Garcia
(feel free to edit the comment with my suggestion, or leave it as it is
if you prefer)
Berto
Re: [Qemu-devel] [PATCH v4 1/3] block: include base when checking image chain for block allocation
09.04.2019 17:18, Alberto Garcia wrote:
> On Mon 08 Apr 2019 08:22:19 PM CEST, Andrey Shinkevich wrote:
>>* Return true if (a prefix of) the given range is allocated in any image
>> - * between BASE and TOP (inclusive). BASE can be NULL to check if the given
>> + * between BASE and TOP (TOP included). To check the BASE image, set the
>> + * 'include_base' to 'true'. The BASE can be NULL to check if the given
>>* offset is allocated in any image of the chain. Return false otherwise,
>>* or negative errno on failure.
>
> I'm not a native speaker but that sounds a bit odd to me.
>
> Alternative:
>
>* Return true if (a prefix of) the given range is allocated in any image
>* between BASE and TOP (BASE is only included if include_base is set).
>* BASE can be NULL to check if the given offset is allocated in any
>* image of the chain. Return false otherwise, or negative errno on
>* failure.
>
>> -while (intermediate && intermediate != base) {
>> +while (include_base || intermediate != base) {
>> int64_t pnum_inter;
>> int64_t size_inter;
>>
>> @@ -2360,6 +2364,10 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>> n = pnum_inter;
>> }
>>
>> +if (intermediate == base) {
>> +break;
>> +}
>> +
>> intermediate = backing_bs(intermediate);
>
> I find that the new condition + the break make things a bit less
> readable. I think it would be simpler with something like this:
>
> BlockDriverState *stop_at = include_base ? backing_bs(base) : base;
>
> while (intermediate != stop_at) {
>...
> }
>
But in this way you return back dependence on base, which we don't freeze and
which
may disappear on some iteration. We should not touch backing_bs(base) in any
way.
--
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH v4 1/3] block: include base when checking image chain for block allocation
On Mon 08 Apr 2019 08:22:19 PM CEST, Andrey Shinkevich wrote:
> * Return true if (a prefix of) the given range is allocated in any image
> - * between BASE and TOP (inclusive). BASE can be NULL to check if the given
> + * between BASE and TOP (TOP included). To check the BASE image, set the
> + * 'include_base' to 'true'. The BASE can be NULL to check if the given
> * offset is allocated in any image of the chain. Return false otherwise,
> * or negative errno on failure.
I'm not a native speaker but that sounds a bit odd to me.
Alternative:
* Return true if (a prefix of) the given range is allocated in any image
* between BASE and TOP (BASE is only included if include_base is set).
* BASE can be NULL to check if the given offset is allocated in any
* image of the chain. Return false otherwise, or negative errno on
* failure.
> -while (intermediate && intermediate != base) {
> +while (include_base || intermediate != base) {
> int64_t pnum_inter;
> int64_t size_inter;
>
> @@ -2360,6 +2364,10 @@ int bdrv_is_allocated_above(BlockDriverState *top,
> n = pnum_inter;
> }
>
> +if (intermediate == base) {
> +break;
> +}
> +
> intermediate = backing_bs(intermediate);
I find that the new condition + the break make things a bit less
readable. I think it would be simpler with something like this:
BlockDriverState *stop_at = include_base ? backing_bs(base) : base;
while (intermediate != stop_at) {
...
}
(stop_at is a terrible name, but I can't think of anything better at the
moment)
Berto
Re: [Qemu-devel] [PATCH v4 1/3] block: include base when checking image chain for block allocation
08.04.2019 21:22, Andrey Shinkevich wrote: > This patch is used in the 'block/stream: introduce a bottom node' > that is following. Instead of the base node, the caller may pass > the node that has the base as its backing image to the function > bdrv_is_allocated_above() with a new parameter include_base = true > and get rid of the dependency on the base that may change during > commit/stream parallel jobs. Now, if the specified base is not > found in the backing image chain, the QEMU will crash. > > Suggested-by: Vladimir Sementsov-Ogievskiy > Signed-off-by: Andrey Shinkevich Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
