[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 7/18/2016 9:07 PM, Andrew Cooper wrote:
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.

Which is then indeed the "unspoken" rule that seemed to be respected throughout the code-base.

One thing that we are currently bad at is having proper distinctions.

Which is why, to also avoid having to go through such back-and-forth discussions, I propose adding this rule in CODING_STYLE. There are, of course, other deviations from it, but doing this would make future deviations less likely and would encourage correcting existing ones.

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.

I agree.

~Andrew

Thanks for the feedback!
Corneliu.

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.