Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 18 Apr 2024, at 07:28, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 09.04.2024 13:45, Luca Fancellu wrote:
>> --- a/xen/arch/x86/extable.c
>> +++ b/xen/arch/x86/extable.c
>> @@ -23,7 +23,8 @@ static inline unsigned long ex_cont(const struct 
>> exception_table_entry *x)
>> return EX_FIELD(x, cont);
>> }
>> 
>> -static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b)
>> +static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b,
>> +                                             const void *data)
>> {
>> const struct exception_table_entry *l = a, *r = b;
>> unsigned long lip = ex_addr(l);
>> @@ -53,7 +54,7 @@ void init_or_livepatch sort_exception_table(struct 
>> exception_table_entry *start,
>>                                  const struct exception_table_entry *stop)
>> {
>>     sort(start, stop - start,
>> -         sizeof(struct exception_table_entry), cmp_ex, swap_ex);
>> +         sizeof(struct exception_table_entry), cmp_ex, swap_ex, NULL);
>> }
> 
> Not the least because of this addition of an entirely useless parameter / 
> argument

Well it’s not useless in this patch, given that without it I couldn’t know the 
size of the address
element, however ...

> I'm not in favor of ...
> 
>> --- a/xen/include/xen/sort.h
>> +++ b/xen/include/xen/sort.h
>> @@ -23,8 +23,8 @@
>> extern gnu_inline
>> #endif
>> void sort(void *base, size_t num, size_t size,
>> -          int (*cmp)(const void *a, const void *b),
>> -          void (*swap)(void *a, void *b, size_t size))
>> +          int (*cmp)(const void *a, const void *b, const void *data),
>> +          void (*swap)(void *a, void *b, size_t size), const void *cmp_data)
>> {
> 
> ... this change. Consider you were doing this on a C library you cannot 
> change.
> You'd have to find a different solution anyway.

I get your point here, we should not change standard functions.

> And the way we have sort()
> right now is matching the C spec. The change to do renders things unexpected 
> to
> anyone wanting to use this function in a spec-compliant way. One approach may
> be to make an adjustment to data representation, such that the extra reference
> data is accessible through the pointers already being passed.
> 
> Jan
> 

Anyway in the end this patch was dropped for other reasons.

Cheers,
Luca