Re: [Xen-devel] [PATCH 03/16] x86/monitor: mechanical renames

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/07/16 08:18, Corneliu ZUZU wrote:
> On 7/12/2016 9:10 AM, Corneliu ZUZU wrote:
>> On 7/11/2016 7:43 PM, Tamas K Lengyel wrote:
>>> On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU
>>> <czuzu@xxxxxxxxxxxxxxx> wrote:
>>>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote:
>>>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU
>>>>> <czuzu@xxxxxxxxxxxxxxx>
>>>>> wrote:
>>>>>> Arch-specific vm-event functions in x86/vm_event.h -e.g.
>>>>>> vm_event_init_domain()-
>>>>>> don't have an 'arch_' prefix. Apply the same rule for monitor
>>>>>> functions -
>>>>>> originally the only two monitor functions that had an 'arch_'
>>>>>> prefix were
>>>>>> arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I
>>>>>> gave them
>>>>>> that
>>>>>> prefix because -they had a counterpart function in common code-,
>>>>>> that
>>>>>> being
>>>>>> monitor_domctl().
>>>>> This should actually be the other way around - ie adding the arch_
>>>>> prefix to vm_event functions that lack it.
>>>>
>>>> Given that the majority of the arch-specific functions called from
>>>> common-code don't have an 'arch_' prefix unless they have a common
>>>> counterpart, I was guessing that was the rule. It made sense in my
>>>> head
>>>> since I saw in that the intention of avoiding naming conflicts (i.e
>>>> you
>>>> can't have monitor_domctl() both on the common-side and on the
>>>> arch-side, so
>>>> prepend 'arch_' to the latter). I noticed you guys also 'skip' the
>>>> prefix
>>>> when sending patches, so that reinforced my assumption.
>>>>
>>>>> Having the arch_ prefix is
>>>>> helpful to know that the function is dealing with the arch specific
>>>>> structs and not common.
>>>>
>>>> Personally I don't see much use in 'knowing that the function is
>>>> dealing
>>>> with the arch structs' from the call-site and you can tell that
>>>> from the
>>>> implementation-site just by looking at the path of its source file.
>>>> Also,
>>>> the code is pretty much localized in the arch directory anyway so
>>>> usually
>>>> one wouldn't have to go back and forth between common and arch that
>>>> often.
>>>> What really bothers me though is always having to read 'arch_' when
>>>> spelling
>>>> a function-name and also that it makes the function name longer
>>>> without much
>>>> benefit. Your suggestion of adding it to pretty much all functions
>>>> that make
>>>> up the interface to common just adds to that headache. :-D
>>>>
>>>>> Similarly that's why we have the hvm_ prefix
>>>>> for functions in hvm/monitor.
>>>>
>>>> 'hvm_'  doesn't seem to me more special than 'monitor_', for
>>>> instance, but
>>>> maybe that's just me.
>>>>
>>>>>> Let this also be the rule for future 'arch_' functions additions,
>>>>>> and
>>>>>> with this
>>>>>> patch remove the 'arch_' prefix from the monitor functions that
>>>>>> don't
>>>>>> have a
>>>>>> counterpart in common-code (all but those 2 aforementioned).
>>>>> Even if there are no common counter-parts to the function, the arch_
>>>>> prefix should remain, so I won't be able to ack this patch.
>>>>>
>>>>> Tamas
>>>>
>>>> Having said the above, are you still of the same opinion?
>>> Yes, I am. While it's not a hard rule to always apply these prefix, it
>>> does make sense to have them so I don't see benefit in removing the
>>> existing prefixes.
>>
>> Well, for one the benefit would be not confusing developers by
>> creating inconsistencies: what's the rule here, i.e. why isn't a
>> function such as alloc_domain_struct prefixed w/ 'arch_' but
>> arch_domain_create is? The reason seems to be the latter having a
>> common counterpart while the former not, at least that's what I see
>> being done all over the code-base. Also, I've done this before and
>> you seemed to agree:
>> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html
>> (Q1). You also suggested creating arch-specific functions without the
>> prefix:
>> https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html
>> . Why the sudden change of heart?
>>
>> 2ndly and obviously, removing the prefixes would make function names
>> shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and
>> then read "vm_event_vcpu_unpause").
>>
>> 3rd reason is that adding the prefix to -all- arch-specific functions
>> called from common would mean having a lot new functions with the
>> prefix. I'd read the prefix over and over again and at some point I'd
>> get annoyed and say "ok, ok, it's arch_, I get it; why use this
>> prefix so much again?".
>>
>> 4th reason is that the advantage of telling that the function
>> accesses arch structures is much too little considering that idk,
>> >50% of the codebase is arch-specific, so it doesn't provide much
>> information, this categorization is too broad to deserve a special
>> prefix. Whereas using the prefix only for functions that do have a
>> common counterpart gives you the extra information that the
>> 'operation' is only -partly- arch-specific, i.e. to see the whole
>> picture, look @ the common-side implementation. Keep in mind that
>> we'd also be -losing that information- if we were to apply the 'go
>> with arch_ for all' rule.. (this could be a 5th reason)
>>
>>> Adding arch_ prefix to the ones that don't already
>>> have one is optional, I was just pointing out that if you really feel
>>> like standardizing the naming convention, that's where I would like
>>> things to move towards to.
>>>
>>> Tamas
>>
>> I don't think I'd say this patch "standardizes the naming convention"
>> but rather "fixes code that doesn't conform to the -already existing-
>> standard naming convention". Above stated reasons explain why I'd
>> rather -not- have this standard go in the direction of adding 'arch_'
>> before a lot of functions.
>>
>> Finally, I feel like asking for feedback as I don't like to insist
>> when the majority agrees to disagree. Jan & others, what's the rule
>> here and what's your view on this? :-)
>>
>> Thanks,
>> Zuzu C.
>
> Can I get some extra feedback on this (Andrew, Stefano, Julien)?

Apologies for the delay.

There is sadly no hard and fast rule.  Generally an arch_$FOO() exists
when there is also a common $FOO() which does some setup, then passes to
arch_$FOO() to do some more architecture specific setup.

One thing that we are currently bad at is having proper distinctions. 
Common code should never ever poke inside a .arch struct/union, and we
should have architecture specific functions to make that happen.

However, I don't think it is wise to insist on an arch_ prefix for every
per-arch helper.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel