On 27/09/2018 13:53, Igor Mammedov wrote: > On Thu, 27 Sep 2018 10:58:47 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> On 27/09/2018 10:47, Igor Mammedov wrote: >>> On Thu, 27 Sep 2018 10:13:07 +0200 >>> David Hildenbrand <da...@redhat.com> wrote: >>> >>>> On 27/09/2018 10:02, Igor Mammedov wrote: >>>>> On Wed, 26 Sep 2018 11:41:59 +0200 >>>>> David Hildenbrand <da...@redhat.com> wrote: >>>>> >>>>>> Make address_space_end point at the real end, instead of end + 1, so we >>>>>> don't >>>>>> have to handle special cases like it being 0. This will allow us to >>>>>> place a memory device at the very end of the guest physical 64bit address >>>>>> space (if ever possible). >>>>> >>>>> [...] >>>>> >>>>>> @@ -115,7 +116,7 @@ uint64_t memory_device_get_free_addr(MachineState >>>>>> *ms, const uint64_t *hint, >>>>>> } >>>>>> address_space_start = ms->device_memory->base; >>>>>> address_space_end = address_space_start + >>>>>> - memory_region_size(&ms->device_memory->mr); >>>>>> + memory_region_size(&ms->device_memory->mr) - 1; >>>>>> >>>>> I'm terrible at math, lets assume size = 1 so after this >>>>> 'real end' would end up at 'address_space_start', which makes >>>>> it rather confusing beside not matching variable name anymore. >>>> >>>> (very simply and unrealistic) counter example as given in the >>>> description. You should get the idea. >>>> >>>> address_space_start = 0xffffffffffffffffull; >>> that should never happen, nor valid in any conditions >>> just add assert so we would catch error if some patch would introduce it >>> >>>> size = 1; >>>> >>>> -> address_space_end = 0; >>>> >>>> While this would be perfectly valid, we would have to handle >>>> address_space_end potentially being 0 in the code below, because this >>>> would be a valid overflow. This, I avoid. >>> assert(address_space_end > address_space_start) >>> would be much better for unrealistic values without messing up with >>> meaning of variables. >>> >>>> And form my POV, the variable name here matches perfectly. It points at >>>> the last address of the address space. (yes people have different >>>> opinions on this) >>> start,end pair is a range, there shouldn't be any other interpretations to >>> this variables. >>> >> >> Please see include/qemu/range.h:struct Range; >> >> So I am using _the definition_. >>>>> >>>>> I'd drop it and maybe extend the following assert to abort >>>>> on overflow condition. >>>> >>>> I'll leave it like this, handling address_space_end = 0 is ugly. >>> Looks like I have to insist on dropping this hunk. >> >> You're going from "I'd" to "I have to insist". Please make up your mind. > my suggestions heavily influenced by the fact that the code should > be easy (for me and hopefully for others) to maintain in future, > when I have to review related code later down the road when original > author is not reachable/care anymore. (i.e. by pure selfishness) > > So far I'm not convinced that this change is for a better hence we stuck > without consensus. > >> I have a R-b from David. But I am willing to change it if you can give >> me a good reason why we should add asserts instead of fixing the code >> (== making it work in all possibly valid scenarios, especially end of >> address space). > if it's about valid cases then it's worth fixing, > but how many of the cases targeted here are in need of fixing in practice? > > I'd keep it simple so it would be readable and do the job > but not clutter it with not necessary logic. > > If we =actually= need to handle every possible permutation, it might > be better to reuse range_foo() helpers you've mentioned, instead of > open coding conditions over again. But only if we really need it or > if it makes current code simpler to read.
We would probably have to add some new range_* helpers for that, something I like to avoid now. So to make some progress, I will keep the changes in this file minimal for now (e.g. specifying a device with a huge size is IMHO possible, so we should fix that). Maybe we can instead focus our energy on the more important parts (namely Patch 18, 23, and 24) - I would really like to hear your opinion about these. Thanks! -- Thanks, David / dhildenb
