Re: [RFC][PATCH 04/13] tools/libxl: detect and avoid conflicts with RDM

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/5/7 14:04, Jan Beulich wrote:
>>>> "Chen, Tiejun" <tiejun.chen@intel.com> 05/07/15 4:22 AM >>>
>> On 2015/5/6 23:34, Jan Beulich wrote:
>>>>>> On 06.05.15 at 17:00, <tiejun.chen@intel.com> wrote:
>>>> On 2015/4/20 19:13, Jan Beulich wrote:
>>>>>>>> On 10.04.15 at 11:21, <tiejun.chen@intel.com> wrote:
>>>>>> +                PERROR("Could not allocate memory.");
>>>>>
>>>>> Now that's exactly the kind of error message that makes no sense:
>>>>> As errno will already cause PERROR() to print something along the
>>>>> lines of the message you provide here, you're just creating
>>>>> redundancy. Indicating the purpose of the allocation, otoh, would
>>>>> add helpful context for the one inspecting the resulting log.
>>>>
>>>> What about this?
>>>>
>>>> PERROR("Could not allocate memory buffers to store reserved device
>>>> memory entries.");
>>>
>>> You kind of go from one extreme to the other - the message
>>> doesn't need to be overly long, but it should be distinct from
>>> all other messages (so that when seen one can identify what
>>> went wrong).
>>
>> I originally refer to some existing examples like this,
>>
>> int
>> xc_core_arch_memory_map_get(xc_interface *xch, struct
>> xc_core_arch_context *unused,
>> xc_dominfo_t *info, shared_info_any_t
>> *live_shinfo,
>> xc_core_memory_map_t **mapp,
>> unsigned int *nr_entries)
>> {
>> ...
>> map = malloc(sizeof(*map));
>> if ( map == NULL )
>> {
>> PERROR("Could not allocate memory");
>> return -1;
>> }
>>
>> Maybe this is wrong to my case. Could I change this?
>
> Yeah, I realize there are bad examples. But we try to avoid spreading those.

Sure.

>
>> PERROR("Could not allocate memory for XENMEM_reserved_device_memory_map
>> hypercall");
>>
>> Or just give me your line.
>
> How about
>
> PERROR("Could not allocate RDM buffer");
>
> It's brief but specific enough to uniquely identify it.

Looks good.

>
>>>>> and hence don't have the final say on stylistic issues, I don't see
>>>>> why the above couldn't be expressed with a single return statement.
>>>>
>>>> Are you saying something like this? Note this was showed by yourself
>>>> long time ago.
>>>
>>> I know, and hence I was puzzled to still see you use the more
>>> convoluted form.
>>>
>>>> static bool check_mmio_hole_conflict(uint64_t start, uint64_t memsize,
>>>>                                          uint64_t mmio_start, uint64_t mmio_size)
>>>> {
>>>>         return start + memsize > mmio_start && start < mmio_start + mmio_size;
>>>> }
>>>>
>>>> But I don't think this really can't work out our case.
>>>
>>> It's equivalent to the original you had, so I don't see what you
>>> mean with "this really can't work out our case".
>>
>> Let me make this point clear.
>>
>> The original implementation,
>>
>> +static int check_rdm_hole(uint64_t start, uint64_t memsize,
>> +                          uint64_t rdm_start, uint64_t rdm_size)
>> +{
>> +    if (start + memsize <= rdm_start || start >= rdm_start + rdm_size)
>> +        return 0;
>> +    else
>> +        return 1;
>> +}
>>
>> means it returns 'false' in two cases:
>>
>> #1. end = start + memsize; end <= rdm_start;
>>
>> This region [start, end] is below of rdm entry.
>>
>> #2. rdm_end = rdm_start + rdm_size; stat >= rdm_end;
>>
>> This region [start, end] is above of rdm entry.
>>
>> So others conditions should indicate that rdm entry is overlapping with
>> this region. Actually this has three cases:
>>
>> #1. This region just conflicts with the first half of rdm entry;
>> #2. This region just conflicts with the second half of rdm entry;
>> #3. This whole region falls inside of rdm entry;
>>
>> Then it should return 'true', right?
>>
>> But with this single line,
>>
>> return start + memsize > rdm_start && start < rdm_start + rdm_size;
>>
>> =>
>>
>> return end > rdm_start && start < rdm_end;
>>
>> This just guarantee it return 'true' *only* if #3 above occurs.
>
> I don't even need to look at all the explanations you give. It is a simple matter
> of expression re-writing to see that
>
>     if (a <= b || c >= d)
>         return 0;
>     else
>         return 1;
>
> is equivalent to
>
>      return !(a <= b || c >= d);
>
> and a simple matter of formal logic to see that this is equivalent to
>
>      return a > b && c < d;

Right now I think you're right.

And I can't recall exactly what's my problem while using this simple 
line, maybe others was misleading me to treat this change as a cause so 
sorry to this confusion.

Thanks
Tiejun