| Message ID | 1468037624-6574-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> 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. Having the arch_ prefix is helpful to know that the function is dealing with the arch specific structs and not common. Similarly that's why we have the hvm_ prefix for functions in hvm/monitor. > > 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
On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: > On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> 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? Zuzu.
On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: >> >> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> >> 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. 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
On 7/11/2016 7:43 PM, Tamas K Lengyel wrote: > On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: >> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: >>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> >>> 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.
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@bitdefender.com> wrote: >>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: >>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <czuzu@bitdefender.com> >>>> 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)? :-) (btw, is Jan on vacation?) Thanks, Corneliu.
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@bitdefender.com> wrote: >>>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: >>>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU >>>>> <czuzu@bitdefender.com> >>>>> 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
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@bitdefender.com> wrote: >>>>> On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: >>>>>> On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU >>>>>> <czuzu@bitdefender.com> >>>>>> 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
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 205df41..043815a 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -22,7 +22,7 @@ #include <asm/monitor.h> #include <public/vm_event.h> -int arch_monitor_init_domain(struct domain *d) +int monitor_init_domain(struct domain *d) { if ( !d->arch.monitor.msr_bitmap ) d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap); @@ -33,7 +33,7 @@ int arch_monitor_init_domain(struct domain *d) return 0; } -void arch_monitor_cleanup_domain(struct domain *d) +void monitor_cleanup_domain(struct domain *d) { xfree(d->arch.monitor.msr_bitmap); diff --git a/xen/common/monitor.c b/xen/common/monitor.c index c73d1d5..5219238 100644 --- a/xen/common/monitor.c +++ b/xen/common/monitor.c @@ -50,12 +50,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) if ( unlikely(mop->event > 31) ) return -EINVAL; /* Check if event type is available. */ - if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) + if ( unlikely(!(monitor_get_capabilities(d) & (1U << mop->event))) ) return -EOPNOTSUPP; break; case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: - mop->event = arch_monitor_get_capabilities(d); + mop->event = monitor_get_capabilities(d); return 0; default: diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 941345b..b4f9fd3 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -669,7 +669,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, { case XEN_VM_EVENT_ENABLE: /* domain_pause() not required here, see XSA-99 */ - rc = arch_monitor_init_domain(d); + rc = monitor_init_domain(d); if ( rc ) break; rc = vm_event_enable(d, vec, ved, _VPF_mem_access, @@ -682,7 +682,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, { domain_pause(d); rc = vm_event_disable(d, ved); - arch_monitor_cleanup_domain(d); + monitor_cleanup_domain(d); domain_unpause(d); } break; diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 4af707a..c72ee42 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -46,20 +46,18 @@ int arch_monitor_domctl_event(struct domain *d, return -EOPNOTSUPP; } -static inline -int arch_monitor_init_domain(struct domain *d) +static inline int monitor_init_domain(struct domain *d) { /* No arch-specific domain initialization on ARM. */ return 0; } -static inline -void arch_monitor_cleanup_domain(struct domain *d) +static inline void monitor_cleanup_domain(struct domain *d) { /* No arch-specific domain cleanup on ARM. */ } -static inline uint32_t arch_monitor_get_capabilities(struct domain *d) +static inline uint32_t monitor_get_capabilities(struct domain *d) { uint32_t capabilities = 0; diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 0501ca2..c5ae7ef 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -60,7 +60,10 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) return rc; } -static inline uint32_t arch_monitor_get_capabilities(struct domain *d) +int arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop); + +static inline uint32_t monitor_get_capabilities(struct domain *d) { uint32_t capabilities = 0; @@ -84,12 +87,9 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) return capabilities; } -int arch_monitor_domctl_event(struct domain *d, - struct xen_domctl_monitor_op *mop); - -int arch_monitor_init_domain(struct domain *d); +int monitor_init_domain(struct domain *d); -void arch_monitor_cleanup_domain(struct domain *d); +void monitor_cleanup_domain(struct domain *d); bool_t monitored_msr(const struct domain *d, u32 msr);
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(). 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). Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/x86/monitor.c | 4 ++-- xen/common/monitor.c | 4 ++-- xen/common/vm_event.c | 4 ++-- xen/include/asm-arm/monitor.h | 8 +++----- xen/include/asm-x86/monitor.h | 12 ++++++------ 5 files changed, 15 insertions(+), 17 deletions(-)