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