Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.10.2019 11:37, Andrew Cooper wrote:
> On 30/10/2019 08:41, Jan Beulich wrote:
>> On 29.10.2019 18:06, Andrew Cooper wrote:
>>> On 24/10/2019 13:03, Jan Beulich wrote:
>>>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -361,9 +361,23 @@ config FAST_SYMBOL_LOOKUP
>>>>>  
>>>>>     If unsure, say Y.
>>>>>  
>>>>> +config ENFORCE_UNIQUE_SYMBOLS
>>>>> + bool "Enforce unique symbols" if LIVEPATCH
>>>>> + default y if LIVEPATCH
>>>> Instead of two identical "if", why not "depends on LIVEPATCH"?
>>>>
>>>>> + ---help---
>>>>> +   Multiple symbols with the same name aren't generally a problem
>>>>> +   unless Live patching is to be used.
>>>>> +
>>>>> +   Livepatch loading involves resolving relocations against symbol
>>>>> +   names, and attempting to a duplicate symbol in a livepatch will
>>>>> +   result in incorrect livepatch application.
>>>>> +
>>>>> +   This option should be used to ensure that a build of Xen can have a
>>>>> +   livepatch build and apply correctly.
>>>>> +
>>>>>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
>>>>> - bool "Suppress duplicate symbol warnings" if !LIVEPATCH
>>>>> - default y if !LIVEPATCH
>>>>> + bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
>>>>> + default y if !ENFORCE_UNIQUE_SYMBOLS
>>>> Similarly here then. With this changed, or with a proper reason
>>>> supplied
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> That's a question for the author of c/s 064a2652233 to answer...  I'm
>>> merely following the prevailing style.
>> "Prevailing style" seems a little bold considering that according to
>> my grep-ing there's exactly on other such instance (VGA). I.e. you'd
>> grow the "badness" by 50% when you could equally well shrink it by
>> this same percentage.
> 
> Allow me to be less subtle.
> 
> *You* are the author of the code, in this form.

I'm sorry for not recalling.

> As a consequence, I expect there is a deliberate reason.
> 
> And seeing as I've had to reverse engineer the reason myself, it looks
> to be related to the fact that other options in the build select these,
> so they need not to be dependent on livepatching in the first place.

It wasn't without reason that I did say "or with a proper reason
supplied" - the select in xen/Kconfig.debug is a proper reason for
SUPPRESS_DUPLICATE_SYMBOL_WARNINGS staying as it is, indeed. But
it's then still not a reason for ENFORCE_UNIQUE_SYMBOLS to be
this same way, as there's no similar select for it anywhere.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel