| Message ID | 20120828160116.GA3047@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 28 August 2012 17:01, Michael S. Tsirkin <mst@redhat.com> wrote: > We copied HACKING from libvirt but it has some bogus stuff: > neither underscore capital, double underscore, or underscore 't' suffixes > are reserved in Posix/C: this appears to be based on misreading of the > C standard. Using sane prefixes is enough to avoid conflicts. > -2.4. Reserved namespaces in C and POSIX > -Underscore capital, double underscore, and underscore 't' suffixes should be > -avoided. I think this is just a missing "prefixes". C99 7.1.3 reserves underscore capital and double underscore prefixes. POSIX reserves _t if any POSIX header is defined (POSIX.1-2008 section 2.2.2, http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) I would suggest that the section is reworded to: # Underscore capital and double underscore prefixes are reserved # by the C standard. Underscore 't' suffixes are reserved by POSIX. # They should be avoided in QEMU code. -- PMM
On Tue, Aug 28, 2012 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > We copied HACKING from libvirt but it has some bogus stuff: > neither underscore capital, double underscore, or underscore 't' suffixes > are reserved in Posix/C: this appears to be based on misreading of the > C standard. Using sane prefixes is enough to avoid conflicts. > > These rules are also widely violated in our codebase, > and it does not make sense to rework it all, apparently for > no benefit. NACK. The benefit is improved standards compliance. One day we could find QEMU being ported to environment which conflicts with these symbols. The tiny single benefit from violating the rules would be that you could use a few additional possible classes of prefixes, in addition to the infinite combinations already available. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > HACKING | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/HACKING b/HACKING > index 471cf1d..0a941fc 100644 > --- a/HACKING > +++ b/HACKING > @@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. > 2.3. Typedefs > Typedefs are used to eliminate the redundant 'struct' keyword. > > -2.4. Reserved namespaces in C and POSIX > -Underscore capital, double underscore, and underscore 't' suffixes should be > -avoided. > - > 3. Low level memory management > > Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign > -- > MST
On Tue, Aug 28, 2012 at 05:24:40PM +0100, Peter Maydell wrote: > On 28 August 2012 17:01, Michael S. Tsirkin <mst@redhat.com> wrote: > > We copied HACKING from libvirt but it has some bogus stuff: > > neither underscore capital, double underscore, or underscore 't' suffixes > > are reserved in Posix/C: this appears to be based on misreading of the > > C standard. Using sane prefixes is enough to avoid conflicts. > > -2.4. Reserved namespaces in C and POSIX > > -Underscore capital, double underscore, and underscore 't' suffixes should be > > -avoided. > > I think this is just a missing "prefixes". C99 7.1.3 > reserves underscore capital and double underscore prefixes. This is taking it out of context - reserved means different things in different parts of the spec. As far as I can see, 7.1.3 talks about what is permissible in headers: naturally when a libc implementation adds stuff in a header it would want to avoid breaking applications including this header. But it does not talk about what is permissible in a legal program - to avoid conflicts, you just need to use variables with reasonable names like kvmXXX qemuXXX or virtioXXX. So it does not look like this is a reason to not use __kvm_pv_eoi_disabled. Chances that a libc header predefines this name are zero. > POSIX reserves _t if any POSIX header is defined (POSIX.1-2008 section > 2.2.2, http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) Same thing really. So this says posix implementations can add types ending with _t in any header. It does not prohibit applications from using such names in a reasonable manner, and it does not look like this is a reason to not use qemu_foo_bar_t. > I would suggest that the section is reworded to: > > # Underscore capital and double underscore prefixes are reserved > # by the C standard. Underscore 't' suffixes are reserved by POSIX. > # They should be avoided in QEMU code. > > -- PMM This might be nice from language purism point of view but ignores the fact that QEMU already uses lots of these. Specifically a _t type is even mentioned in HACKING itself. So the benefit to excluding this in new code is marginal.
On Tue, Aug 28, 2012 at 05:13:24PM +0000, Blue Swirl wrote: > On Tue, Aug 28, 2012 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > We copied HACKING from libvirt but it has some bogus stuff: > > neither underscore capital, double underscore, or underscore 't' suffixes > > are reserved in Posix/C: this appears to be based on misreading of the > > C standard. Using sane prefixes is enough to avoid conflicts. > > > > These rules are also widely violated in our codebase, > > and it does not make sense to rework it all, apparently for > > no benefit. > > NACK. The benefit is improved standards compliance. One day we could > find QEMU being ported to environment which conflicts with these > symbols. We are talking about stuff like __kvm_pv_eoi - so the chance is exactly 0. And if it does happen then you run a simple script and fix this one instance. > The tiny single benefit from violating the rules would be that you > could use a few additional possible classes of prefixes, in addition > to the infinite combinations already available. Benefit would be consistency with existing QEMU code which has both _t __ and _X, and consistency within HACKING itself. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > HACKING | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/HACKING b/HACKING > > index 471cf1d..0a941fc 100644 > > --- a/HACKING > > +++ b/HACKING > > @@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. > > 2.3. Typedefs > > Typedefs are used to eliminate the redundant 'struct' keyword. > > > > -2.4. Reserved namespaces in C and POSIX > > -Underscore capital, double underscore, and underscore 't' suffixes should be > > -avoided. > > - > > 3. Low level memory management > > > > Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign > > -- > > MST
On 28 August 2012 18:18, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Aug 28, 2012 at 05:24:40PM +0100, Peter Maydell wrote: >> C99 7.1.3 >> reserves underscore capital and double underscore prefixes. > > This is taking it out of context - reserved means different > things in different parts of the spec. > > As far as I can see, 7.1.3 talks about what is permissible in headers: > naturally when a libc implementation adds stuff in a header it would > want to avoid breaking applications including this header. But > it does not talk about what is permissible in a legal program - to avoid > conflicts, you just need to use variables with reasonable names like > kvmXXX qemuXXX or virtioXXX. 7.1.3 para 2 says "If the program declares or defines an identifier in a context in which it is reserved [...] the behavior is undefined." The rationale for 7.1.3 says that the underscore-cap and double underscore identifiers are "reserved for the implementor" and that "part of the name space of internal identifiers beginning with underscore is available to the user" which implies that the rest of that namespace is *not* available to the user. I think that's fairly clear that we can't use these identifiers without entering the realm of undefined behavior. -- PMM
On Tue, Aug 28, 2012 at 5:21 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Aug 28, 2012 at 05:13:24PM +0000, Blue Swirl wrote: >> On Tue, Aug 28, 2012 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > We copied HACKING from libvirt but it has some bogus stuff: >> > neither underscore capital, double underscore, or underscore 't' suffixes >> > are reserved in Posix/C: this appears to be based on misreading of the >> > C standard. Using sane prefixes is enough to avoid conflicts. >> > >> > These rules are also widely violated in our codebase, >> > and it does not make sense to rework it all, apparently for >> > no benefit. >> >> NACK. The benefit is improved standards compliance. One day we could >> find QEMU being ported to environment which conflicts with these >> symbols. > > We are talking about stuff like __kvm_pv_eoi - so the chance is exactly 0. How do you know for sure? The implementation could be that symbols are converted by the compiler to use underscore prefixes. > And if it does happen then you run a simple script and fix > this one instance. Then why not fix now? How about kvm_pv_eoi_state? > >> The tiny single benefit from violating the rules would be that you >> could use a few additional possible classes of prefixes, in addition >> to the infinite combinations already available. > > Benefit would be consistency with existing QEMU code > which has both _t __ and _X, and consistency > within HACKING itself. There are plenty of bad things wrt style, but the plan is to get rid of those eventually. It takes time since global reformatting etc. to enforce the rules is not accepted. > >> > >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> > --- >> > HACKING | 4 ---- >> > 1 file changed, 4 deletions(-) >> > >> > diff --git a/HACKING b/HACKING >> > index 471cf1d..0a941fc 100644 >> > --- a/HACKING >> > +++ b/HACKING >> > @@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. >> > 2.3. Typedefs >> > Typedefs are used to eliminate the redundant 'struct' keyword. >> > >> > -2.4. Reserved namespaces in C and POSIX >> > -Underscore capital, double underscore, and underscore 't' suffixes should be >> > -avoided. >> > - >> > 3. Low level memory management >> > >> > Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign >> > -- >> > MST
On 28 August 2012 18:21, Michael S. Tsirkin <mst@redhat.com> wrote: > We are talking about stuff like __kvm_pv_eoi - so the chance is exactly 0. > And if it does happen then you run a simple script and fix > this one instance. Why not just use a name that doesn't use a double underscore in the first place? The C standard specifically allows single underscore + lowercase to give things other than the implementation part of the underscore-namespace. In this case, "_kvm_pv_eoi" would be OK. >> The tiny single benefit from violating the rules would be that you >> could use a few additional possible classes of prefixes, in addition >> to the infinite combinations already available. > > Benefit would be consistency with existing QEMU code > which has both _t __ and _X, and consistency > within HACKING itself. HACKING and CODING_STYLE contain a number of rules which the existing codebase doesn't fully conform to. The idea is to incrementally improve consistency and correctness. -- PMM
On Tue, 28 Aug 2012, Michael S. Tsirkin wrote: > On Tue, Aug 28, 2012 at 05:24:40PM +0100, Peter Maydell wrote: > > On 28 August 2012 17:01, Michael S. Tsirkin <mst@redhat.com> wrote: > > > We copied HACKING from libvirt but it has some bogus stuff: > > > neither underscore capital, double underscore, or underscore 't' suffixes > > > are reserved in Posix/C: this appears to be based on misreading of the > > > C standard. Using sane prefixes is enough to avoid conflicts. > > > > -2.4. Reserved namespaces in C and POSIX > > > -Underscore capital, double underscore, and underscore 't' suffixes should be > > > -avoided. > > > > I think this is just a missing "prefixes". C99 7.1.3 > > reserves underscore capital and double underscore prefixes. > > This is taking it out of context - reserved means different > things in different parts of the spec. Reserved means - reserved for implementation to use as it sees fit, for instance - 6.2.5 28) Implementation-defined keywords shall have the form of an identifier reserved for any use as described in 7.1.3. Future versions of C might define something in the form __xxx or _[A-Z]xxx precisely because those are invalid for user to define.. (_Imaginary, _Bool, whatever) 7.1.3#2 > > As far as I can see, 7.1.3 talks about what is permissible in headers: And you are wrong. > naturally when a libc implementation adds stuff in a header it would > want to avoid breaking applications including this header. But it does > not talk about what is permissible in a legal program - to avoid > conflicts, you just need to use variables with reasonable names like > kvmXXX qemuXXX or virtioXXX. > > So it does not look like this is a reason to not use __kvm_pv_eoi_disabled. > > Chances that a libc header predefines this name are zero. > > > POSIX reserves _t if any POSIX header is defined (POSIX.1-2008 section > > 2.2.2, http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html) > > Same thing really. > So this says posix implementations can add types ending with _t in any > header. It does not prohibit applications from using such names > in a reasonable manner, and it does not look like this is a reason to > not use qemu_foo_bar_t. > > > > I would suggest that the section is reworded to: > > > > # Underscore capital and double underscore prefixes are reserved > > # by the C standard. Underscore 't' suffixes are reserved by POSIX. > > # They should be avoided in QEMU code. > > > > -- PMM > > This might be nice from language purism point of view > but ignores the fact that QEMU already uses lots of these. > Specifically a _t type is even mentioned in HACKING itself. > > So the benefit to excluding this in new code is marginal. > > >
On Tue, Aug 28, 2012 at 06:23:38PM +0100, Peter Maydell wrote: > On 28 August 2012 18:18, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Aug 28, 2012 at 05:24:40PM +0100, Peter Maydell wrote: > >> C99 7.1.3 > >> reserves underscore capital and double underscore prefixes. > > > > This is taking it out of context - reserved means different > > things in different parts of the spec. > > > > As far as I can see, 7.1.3 talks about what is permissible in headers: > > naturally when a libc implementation adds stuff in a header it would > > want to avoid breaking applications including this header. But > > it does not talk about what is permissible in a legal program - to avoid > > conflicts, you just need to use variables with reasonable names like > > kvmXXX qemuXXX or virtioXXX. > > 7.1.3 para 2 says "If the program declares or defines an identifier > in a context in which it is reserved [...] the behavior is undefined." > The rationale for 7.1.3 says that the underscore-cap and double > underscore identifiers are "reserved for the implementor" and that > "part of the name space of internal identifiers beginning with > underscore is available to the user" which implies that the rest > of that namespace is *not* available to the user. > > I think that's fairly clear that we can't use these identifiers > without entering the realm of undefined behavior. > I'll have to re-read that, I missed it. What about _t in POSIX? That seems fairly safe if name is long and qemu specific enough.
On Tue, 28 Aug 2012, Peter Maydell wrote: > On 28 August 2012 18:21, Michael S. Tsirkin <mst@redhat.com> wrote: > > We are talking about stuff like __kvm_pv_eoi - so the chance is exactly 0. > > And if it does happen then you run a simple script and fix > > this one instance. > > Why not just use a name that doesn't use a double underscore > in the first place? The C standard specifically allows single > underscore + lowercase to give things other than the implementation > part of the underscore-namespace. In this case, "_kvm_pv_eoi" > would be OK. No it wouldn't, _kvm_pv_eoi is a file scope identifier, and names beginning with underscore are reserved in this context. > > >> The tiny single benefit from violating the rules would be that you > >> could use a few additional possible classes of prefixes, in addition > >> to the infinite combinations already available. > > > > Benefit would be consistency with existing QEMU code > > which has both _t __ and _X, and consistency > > within HACKING itself. > > HACKING and CODING_STYLE contain a number of rules which > the existing codebase doesn't fully conform to. The idea > is to incrementally improve consistency and correctness. > > -- PMM >
Am 28.08.2012 18:01, schrieb Michael S. Tsirkin: > We copied HACKING from libvirt but it has some bogus stuff: > neither underscore capital, double underscore, or underscore 't' suffixes > are reserved in Posix/C: this appears to be based on misreading of the > C standard. Using sane prefixes is enough to avoid conflicts. > > These rules are also widely violated in our codebase, > and it does not make sense to rework it all, apparently for > no benefit. It was generally not accepted to do Coding Style cleanups in the code base so you will find lots of violations for any rule if you invest the time to search. That's no reason to abolish any such rule. Cc'ing malc, who has "violently" fought for this particular rule. I also think that asking people not to introduce new ..._t typedefs is different from asking to use appropriate existing typedefs, so HACKING is not really contradicting itself in that aspect. Regards, Andreas > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > HACKING | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/HACKING b/HACKING > index 471cf1d..0a941fc 100644 > --- a/HACKING > +++ b/HACKING > @@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. > 2.3. Typedefs > Typedefs are used to eliminate the redundant 'struct' keyword. > > -2.4. Reserved namespaces in C and POSIX > -Underscore capital, double underscore, and underscore 't' suffixes should be > -avoided. > - > 3. Low level memory management > > Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign >
On Tue, Aug 28, 2012 at 06:27:59PM +0100, Peter Maydell wrote: > On 28 August 2012 18:21, Michael S. Tsirkin <mst@redhat.com> wrote: > > We are talking about stuff like __kvm_pv_eoi - so the chance is exactly 0. > > And if it does happen then you run a simple script and fix > > this one instance. > > Why not just use a name that doesn't use a double underscore > in the first place? The C standard specifically allows single > underscore + lowercase to give things other than the implementation > part of the underscore-namespace. In this case, "_kvm_pv_eoi" > would be OK. BTW this is exactly what v2 of my patch did but Blue Swirl nacked this too. > >> The tiny single benefit from violating the rules would be that you > >> could use a few additional possible classes of prefixes, in addition > >> to the infinite combinations already available. > > > > Benefit would be consistency with existing QEMU code > > which has both _t __ and _X, and consistency > > within HACKING itself. > > HACKING and CODING_STYLE contain a number of rules which > the existing codebase doesn't fully conform to. The idea > is to incrementally improve consistency and correctness. > > -- PMM How about fixing HACKING itself? It recommends using ram_addr_t.
On Tue, Aug 28, 2012 at 09:33:16PM +0400, malc wrote: > On Tue, 28 Aug 2012, Peter Maydell wrote: > > > On 28 August 2012 18:21, Michael S. Tsirkin <mst@redhat.com> wrote: > > > We are talking about stuff like __kvm_pv_eoi - so the chance is exactly 0. > > > And if it does happen then you run a simple script and fix > > > this one instance. > > > > Why not just use a name that doesn't use a double underscore > > in the first place? The C standard specifically allows single > > underscore + lowercase to give things other than the implementation > > part of the underscore-namespace. In this case, "_kvm_pv_eoi" > > would be OK. > > No it wouldn't, _kvm_pv_eoi is a file scope identifier, and names > beginning with underscore are reserved in this context. Looks like they are and I missed that. Maybe we should add that to HACKING. > > > > >> The tiny single benefit from violating the rules would be that you > > >> could use a few additional possible classes of prefixes, in addition > > >> to the infinite combinations already available. > > > > > > Benefit would be consistency with existing QEMU code > > > which has both _t __ and _X, and consistency > > > within HACKING itself. > > > > HACKING and CODING_STYLE contain a number of rules which > > the existing codebase doesn't fully conform to. The idea > > is to incrementally improve consistency and correctness. > > > > -- PMM > > > > -- > mailto:av1474@comtv.ru
On 28 August 2012 18:32, Michael S. Tsirkin <mst@redhat.com> wrote: > What about _t in POSIX? That seems fairly safe if name is long and qemu > specific enough. Depends what you mean by "safe". The spec says "don't use this"; it isn't any different to the __ and _[A-Z] prohibitions in that respect. Other posix namespace landgrabs you may not have expected: * ctype.h reserves "is[a-z]" and to[a-z]" prefixes * string.h takes "str[a-z]" and "mem[a-z]" prefixes (and qemu-common.h includes both string.h and ctype.h so this effectively applies to all of qemu). I'm in two minds about these, because on the one hand they're reserved but on the other hand abiding by the rules makes things uglier (whereas "use one underscore not two" doesn't produce an uglier name IMHO). (posix also if I'm reading it correctly reserves all of 'prefix underscore', which is annoying.) -- PMM
On Tue, Aug 28, 2012 at 06:46:53PM +0100, Peter Maydell wrote: > On 28 August 2012 18:32, Michael S. Tsirkin <mst@redhat.com> wrote: > > What about _t in POSIX? That seems fairly safe if name is long and qemu > > specific enough. > > Depends what you mean by "safe". The spec says "don't use this"; > it isn't any different to the __ and _[A-Z] prohibitions in that > respect. It's different: unlike C compiler POSIX can not mangle names in an application. So it will not internally create qemu_foo_bar_t from qemu_foo_bar, and thus qemu_foo_bar_t is very unlikely to create conflicts. OTOH compiler people do mysterious and strange things to the point where you do not want to mess with them if you can. > Other posix namespace landgrabs you may not have expected: > * ctype.h reserves "is[a-z]" and to[a-z]" prefixes > * string.h takes "str[a-z]" and "mem[a-z]" prefixes > > (and qemu-common.h includes both string.h and ctype.h so this > effectively applies to all of qemu). > > I'm in two minds about these, because on the one hand they're > reserved but on the other hand abiding by the rules makes things > uglier (whereas "use one underscore not two" doesn't produce > an uglier name IMHO). > > (posix also if I'm reading it correctly reserves all of 'prefix > underscore', which is annoying.) > > -- PMM Exactly my points.
diff --git a/HACKING b/HACKING index 471cf1d..0a941fc 100644 --- a/HACKING +++ b/HACKING @@ -69,10 +69,6 @@ it points to, or it is aliased to another pointer that is. 2.3. Typedefs Typedefs are used to eliminate the redundant 'struct' keyword. -2.4. Reserved namespaces in C and POSIX -Underscore capital, double underscore, and underscore 't' suffixes should be -avoided. - 3. Low level memory management Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
We copied HACKING from libvirt but it has some bogus stuff: neither underscore capital, double underscore, or underscore 't' suffixes are reserved in Posix/C: this appears to be based on misreading of the C standard. Using sane prefixes is enough to avoid conflicts. These rules are also widely violated in our codebase, and it does not make sense to rework it all, apparently for no benefit. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- HACKING | 4 ---- 1 file changed, 4 deletions(-)