| Message ID | 1323879637-16901-2-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 14 Dec 2011, Anthony Liguori wrote: > GTK/glib uses a convenient of: > > typedef struct _CamelCase CamelCase; > > The reason that they use a separate struct name is that in C++, the struct > namespace not a separate namespace from the type namespace. This is actually a > reasonable policy for QEMU to adopt as we eventually start exporting C libraries > that may be consumed by C++ programs. > > I think the use of _ does not violate the C specification as the struct > namespace is not the same as the type namespace which is what the C spec refers > to if I understand it correctly. It does violate the standard _ followed by upper case letter is reserved in all contexts. > > Additionally, gtkdoc-scan cannot handle nested structs so remove those from the > memory API. > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > ioport.h | 14 ++++---- > memory.c | 6 ++-- > memory.h | 99 +++++++++++++++++++++++++++++++++---------------------------- > 3 files changed, 64 insertions(+), 55 deletions(-) > > diff --git a/ioport.h b/ioport.h > index ae3e9da..99345d8 100644 > --- a/ioport.h > +++ b/ioport.h > @@ -52,24 +52,24 @@ uint8_t cpu_inb(pio_addr_t addr); > uint16_t cpu_inw(pio_addr_t addr); > uint32_t cpu_inl(pio_addr_t addr); > > -struct MemoryRegion; > -struct MemoryRegionPortio; > +struct _MemoryRegion; > +struct _MemoryRegionPortio; > > typedef struct PortioList { > - const struct MemoryRegionPortio *ports; > - struct MemoryRegion *address_space; > + const struct _MemoryRegionPortio *ports; > + struct _MemoryRegion *address_space; > unsigned nr; > - struct MemoryRegion **regions; > + struct _MemoryRegion **regions; > void *opaque; > const char *name; > } PortioList; > > void portio_list_init(PortioList *piolist, > - const struct MemoryRegionPortio *callbacks, > + const struct _MemoryRegionPortio *callbacks, > void *opaque, const char *name); > void portio_list_destroy(PortioList *piolist); > void portio_list_add(PortioList *piolist, > - struct MemoryRegion *address_space, > + struct _MemoryRegion *address_space, > uint32_t addr); > void portio_list_del(PortioList *piolist); > > diff --git a/memory.c b/memory.c > index adfdf14..76a7ae6 100644 > --- a/memory.c > +++ b/memory.c > @@ -72,12 +72,12 @@ static AddrRange addrrange_intersection(AddrRange r1, AddrRange r2) > return addrrange_make(start, int128_sub(end, start)); > } > > -struct CoalescedMemoryRange { > +struct _CoalescedMemoryRange { > AddrRange addr; > - QTAILQ_ENTRY(CoalescedMemoryRange) link; > + QTAILQ_ENTRY(_CoalescedMemoryRange) link; > }; > > -struct MemoryRegionIoeventfd { > +struct _MemoryRegionIoeventfd { > AddrRange addr; > bool match_data; > uint64_t data; > diff --git a/memory.h b/memory.h > index beae127..3aa8404 100644 > --- a/memory.h > +++ b/memory.h > @@ -26,10 +26,12 @@ > #include "ioport.h" > #include "int128.h" > > -typedef struct MemoryRegionOps MemoryRegionOps; > -typedef struct MemoryRegion MemoryRegion; > -typedef struct MemoryRegionPortio MemoryRegionPortio; > -typedef struct MemoryRegionMmio MemoryRegionMmio; > +typedef struct _MemoryRegionOps MemoryRegionOps; > +typedef struct _MemoryRegion MemoryRegion; > +typedef struct _MemoryRegionPortio MemoryRegionPortio; > +typedef struct _MemoryRegionMmio MemoryRegionMmio; > +typedef struct _MemoryRegionGuestConstraints MemoryRegionGuestConstraints; > +typedef struct _MemoryRegionInternalConstraints MemoryRegionInternalConstraints; > > /* Must match *_DIRTY_FLAGS in cpu-all.h. To be replaced with dynamic > * registration. > @@ -38,15 +40,51 @@ typedef struct MemoryRegionMmio MemoryRegionMmio; > #define DIRTY_MEMORY_CODE 1 > #define DIRTY_MEMORY_MIGRATION 3 > > -struct MemoryRegionMmio { > +struct _MemoryRegionMmio { > CPUReadMemoryFunc *read[3]; > CPUWriteMemoryFunc *write[3]; > }; > > +struct _MemoryRegionGuestConstraints > +{ > + /* If nonzero, specify bounds on access sizes beyond which a machine > + * check is thrown. > + */ > + unsigned min_access_size; > + unsigned max_access_size; > + /* If true, unaligned accesses are supported. Otherwise unaligned > + * accesses throw machine checks. > + */ > + bool unaligned; > + /* > + * If present, and returns #false, the transaction is not accepted > + * by the device (and results in machine dependent behaviour such > + * as a machine check exception). > + */ > + bool (*accepts)(void *opaque, target_phys_addr_t addr, > + unsigned size, bool is_write); > +}; > + > +struct _MemoryRegionInternalConstraints > +{ > + /* If nonzero, specifies the minimum size implemented. Smaller sizes > + * will be rounded upwards and a partial result will be returned. > + */ > + unsigned min_access_size; > + /* If nonzero, specifies the maximum size implemented. Larger sizes > + * will be done as a series of accesses with smaller sizes. > + */ > + unsigned max_access_size; > + /* If true, unaligned accesses are supported. Otherwise all accesses > + * are converted to (possibly multiple) naturally aligned accesses. > + */ > + bool unaligned; > +}; > + > /* > * Memory region callbacks > */ > -struct MemoryRegionOps { > +struct _MemoryRegionOps { > /* Read from the memory region. @addr is relative to @mr; @size is > * in bytes. */ > uint64_t (*read)(void *opaque, > @@ -61,39 +99,10 @@ struct MemoryRegionOps { > > enum device_endian endianness; > /* Guest-visible constraints: */ > - struct { > - /* If nonzero, specify bounds on access sizes beyond which a machine > - * check is thrown. > - */ > - unsigned min_access_size; > - unsigned max_access_size; > - /* If true, unaligned accesses are supported. Otherwise unaligned > - * accesses throw machine checks. > - */ > - bool unaligned; > - /* > - * If present, and returns #false, the transaction is not accepted > - * by the device (and results in machine dependent behaviour such > - * as a machine check exception). > - */ > - bool (*accepts)(void *opaque, target_phys_addr_t addr, > - unsigned size, bool is_write); > - } valid; > + MemoryRegionGuestConstraints valid; > + > /* Internal implementation constraints: */ > - struct { > - /* If nonzero, specifies the minimum size implemented. Smaller sizes > - * will be rounded upwards and a partial result will be returned. > - */ > - unsigned min_access_size; > - /* If nonzero, specifies the maximum size implemented. Larger sizes > - * will be done as a series of accesses with smaller sizes. > - */ > - unsigned max_access_size; > - /* If true, unaligned accesses are supported. Otherwise all accesses > - * are converted to (possibly multiple) naturally aligned accesses. > - */ > - bool unaligned; > - } impl; > + MemoryRegionInternalConstraints impl; > > /* If .read and .write are not present, old_portio may be used for > * backwards compatibility with old portio registration > @@ -105,10 +114,10 @@ struct MemoryRegionOps { > const MemoryRegionMmio old_mmio; > }; > > -typedef struct CoalescedMemoryRange CoalescedMemoryRange; > -typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; > +typedef struct _CoalescedMemoryRange CoalescedMemoryRange; > +typedef struct _MemoryRegionIoeventfd MemoryRegionIoeventfd; > > -struct MemoryRegion { > +struct _MemoryRegion { > /* All fields are private - violators will be prosecuted */ > const MemoryRegionOps *ops; > void *opaque; > @@ -127,16 +136,16 @@ struct MemoryRegion { > target_phys_addr_t alias_offset; > unsigned priority; > bool may_overlap; > - QTAILQ_HEAD(subregions, MemoryRegion) subregions; > - QTAILQ_ENTRY(MemoryRegion) subregions_link; > - QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced; > + QTAILQ_HEAD(subregions, _MemoryRegion) subregions; > + QTAILQ_ENTRY(_MemoryRegion) subregions_link; > + QTAILQ_HEAD(coalesced_ranges, _CoalescedMemoryRange) coalesced; > const char *name; > uint8_t dirty_log_mask; > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > }; > > -struct MemoryRegionPortio { > +struct _MemoryRegionPortio { > uint32_t offset; > uint32_t len; > unsigned size; >
On 12/14/2011 10:34 AM, malc wrote: > On Wed, 14 Dec 2011, Anthony Liguori wrote: > >> GTK/glib uses a convenient of: >> >> typedef struct _CamelCase CamelCase; >> >> The reason that they use a separate struct name is that in C++, the struct >> namespace not a separate namespace from the type namespace. This is actually a >> reasonable policy for QEMU to adopt as we eventually start exporting C libraries >> that may be consumed by C++ programs. >> >> I think the use of _ does not violate the C specification as the struct >> namespace is not the same as the type namespace which is what the C spec refers >> to if I understand it correctly. > > It does violate the standard _ followed by upper case letter is reserved > in all contexts. I think in this case, we're just going to have to deviate from the standard. The benefit is great, the practice is widespread, and there's no immediate likelihood of changing glib's coding style practices. I still think we should adhere to this policy where ever possible but I think we'll have to make an exception for extracting documentation. Regards, Anthony Liguori >> >> Additionally, gtkdoc-scan cannot handle nested structs so remove those from the >> memory API. >> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> --- >> ioport.h | 14 ++++---- >> memory.c | 6 ++-- >> memory.h | 99 +++++++++++++++++++++++++++++++++---------------------------- >> 3 files changed, 64 insertions(+), 55 deletions(-) >> >> diff --git a/ioport.h b/ioport.h >> index ae3e9da..99345d8 100644 >> --- a/ioport.h >> +++ b/ioport.h >> @@ -52,24 +52,24 @@ uint8_t cpu_inb(pio_addr_t addr); >> uint16_t cpu_inw(pio_addr_t addr); >> uint32_t cpu_inl(pio_addr_t addr); >> >> -struct MemoryRegion; >> -struct MemoryRegionPortio; >> +struct _MemoryRegion; >> +struct _MemoryRegionPortio; >> >> typedef struct PortioList { >> - const struct MemoryRegionPortio *ports; >> - struct MemoryRegion *address_space; >> + const struct _MemoryRegionPortio *ports; >> + struct _MemoryRegion *address_space; >> unsigned nr; >> - struct MemoryRegion **regions; >> + struct _MemoryRegion **regions; >> void *opaque; >> const char *name; >> } PortioList; >> >> void portio_list_init(PortioList *piolist, >> - const struct MemoryRegionPortio *callbacks, >> + const struct _MemoryRegionPortio *callbacks, >> void *opaque, const char *name); >> void portio_list_destroy(PortioList *piolist); >> void portio_list_add(PortioList *piolist, >> - struct MemoryRegion *address_space, >> + struct _MemoryRegion *address_space, >> uint32_t addr); >> void portio_list_del(PortioList *piolist); >> >> diff --git a/memory.c b/memory.c >> index adfdf14..76a7ae6 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -72,12 +72,12 @@ static AddrRange addrrange_intersection(AddrRange r1, AddrRange r2) >> return addrrange_make(start, int128_sub(end, start)); >> } >> >> -struct CoalescedMemoryRange { >> +struct _CoalescedMemoryRange { >> AddrRange addr; >> - QTAILQ_ENTRY(CoalescedMemoryRange) link; >> + QTAILQ_ENTRY(_CoalescedMemoryRange) link; >> }; >> >> -struct MemoryRegionIoeventfd { >> +struct _MemoryRegionIoeventfd { >> AddrRange addr; >> bool match_data; >> uint64_t data; >> diff --git a/memory.h b/memory.h >> index beae127..3aa8404 100644 >> --- a/memory.h >> +++ b/memory.h >> @@ -26,10 +26,12 @@ >> #include "ioport.h" >> #include "int128.h" >> >> -typedef struct MemoryRegionOps MemoryRegionOps; >> -typedef struct MemoryRegion MemoryRegion; >> -typedef struct MemoryRegionPortio MemoryRegionPortio; >> -typedef struct MemoryRegionMmio MemoryRegionMmio; >> +typedef struct _MemoryRegionOps MemoryRegionOps; >> +typedef struct _MemoryRegion MemoryRegion; >> +typedef struct _MemoryRegionPortio MemoryRegionPortio; >> +typedef struct _MemoryRegionMmio MemoryRegionMmio; >> +typedef struct _MemoryRegionGuestConstraints MemoryRegionGuestConstraints; >> +typedef struct _MemoryRegionInternalConstraints MemoryRegionInternalConstraints; >> >> /* Must match *_DIRTY_FLAGS in cpu-all.h. To be replaced with dynamic >> * registration. >> @@ -38,15 +40,51 @@ typedef struct MemoryRegionMmio MemoryRegionMmio; >> #define DIRTY_MEMORY_CODE 1 >> #define DIRTY_MEMORY_MIGRATION 3 >> >> -struct MemoryRegionMmio { >> +struct _MemoryRegionMmio { >> CPUReadMemoryFunc *read[3]; >> CPUWriteMemoryFunc *write[3]; >> }; >> >> +struct _MemoryRegionGuestConstraints >> +{ >> + /* If nonzero, specify bounds on access sizes beyond which a machine >> + * check is thrown. >> + */ >> + unsigned min_access_size; >> + unsigned max_access_size; >> + /* If true, unaligned accesses are supported. Otherwise unaligned >> + * accesses throw machine checks. >> + */ >> + bool unaligned; >> + /* >> + * If present, and returns #false, the transaction is not accepted >> + * by the device (and results in machine dependent behaviour such >> + * as a machine check exception). >> + */ >> + bool (*accepts)(void *opaque, target_phys_addr_t addr, >> + unsigned size, bool is_write); >> +}; >> + >> +struct _MemoryRegionInternalConstraints >> +{ >> + /* If nonzero, specifies the minimum size implemented. Smaller sizes >> + * will be rounded upwards and a partial result will be returned. >> + */ >> + unsigned min_access_size; >> + /* If nonzero, specifies the maximum size implemented. Larger sizes >> + * will be done as a series of accesses with smaller sizes. >> + */ >> + unsigned max_access_size; >> + /* If true, unaligned accesses are supported. Otherwise all accesses >> + * are converted to (possibly multiple) naturally aligned accesses. >> + */ >> + bool unaligned; >> +}; >> + >> /* >> * Memory region callbacks >> */ >> -struct MemoryRegionOps { >> +struct _MemoryRegionOps { >> /* Read from the memory region. @addr is relative to @mr; @size is >> * in bytes. */ >> uint64_t (*read)(void *opaque, >> @@ -61,39 +99,10 @@ struct MemoryRegionOps { >> >> enum device_endian endianness; >> /* Guest-visible constraints: */ >> - struct { >> - /* If nonzero, specify bounds on access sizes beyond which a machine >> - * check is thrown. >> - */ >> - unsigned min_access_size; >> - unsigned max_access_size; >> - /* If true, unaligned accesses are supported. Otherwise unaligned >> - * accesses throw machine checks. >> - */ >> - bool unaligned; >> - /* >> - * If present, and returns #false, the transaction is not accepted >> - * by the device (and results in machine dependent behaviour such >> - * as a machine check exception). >> - */ >> - bool (*accepts)(void *opaque, target_phys_addr_t addr, >> - unsigned size, bool is_write); >> - } valid; >> + MemoryRegionGuestConstraints valid; >> + >> /* Internal implementation constraints: */ >> - struct { >> - /* If nonzero, specifies the minimum size implemented. Smaller sizes >> - * will be rounded upwards and a partial result will be returned. >> - */ >> - unsigned min_access_size; >> - /* If nonzero, specifies the maximum size implemented. Larger sizes >> - * will be done as a series of accesses with smaller sizes. >> - */ >> - unsigned max_access_size; >> - /* If true, unaligned accesses are supported. Otherwise all accesses >> - * are converted to (possibly multiple) naturally aligned accesses. >> - */ >> - bool unaligned; >> - } impl; >> + MemoryRegionInternalConstraints impl; >> >> /* If .read and .write are not present, old_portio may be used for >> * backwards compatibility with old portio registration >> @@ -105,10 +114,10 @@ struct MemoryRegionOps { >> const MemoryRegionMmio old_mmio; >> }; >> >> -typedef struct CoalescedMemoryRange CoalescedMemoryRange; >> -typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; >> +typedef struct _CoalescedMemoryRange CoalescedMemoryRange; >> +typedef struct _MemoryRegionIoeventfd MemoryRegionIoeventfd; >> >> -struct MemoryRegion { >> +struct _MemoryRegion { >> /* All fields are private - violators will be prosecuted */ >> const MemoryRegionOps *ops; >> void *opaque; >> @@ -127,16 +136,16 @@ struct MemoryRegion { >> target_phys_addr_t alias_offset; >> unsigned priority; >> bool may_overlap; >> - QTAILQ_HEAD(subregions, MemoryRegion) subregions; >> - QTAILQ_ENTRY(MemoryRegion) subregions_link; >> - QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced; >> + QTAILQ_HEAD(subregions, _MemoryRegion) subregions; >> + QTAILQ_ENTRY(_MemoryRegion) subregions_link; >> + QTAILQ_HEAD(coalesced_ranges, _CoalescedMemoryRange) coalesced; >> const char *name; >> uint8_t dirty_log_mask; >> unsigned ioeventfd_nb; >> MemoryRegionIoeventfd *ioeventfds; >> }; >> >> -struct MemoryRegionPortio { >> +struct _MemoryRegionPortio { >> uint32_t offset; >> uint32_t len; >> unsigned size; >> >
On 14 December 2011 16:55, Anthony Liguori <anthony@codemonkey.ws> wrote: > I think in this case, we're just going to have to deviate from the standard. > The benefit is great, the practice is widespread, and there's no immediate > likelihood of changing glib's coding style practices. > > I still think we should adhere to this policy where ever possible but I > think we'll have to make an exception for extracting documentation. Disagree. A docs tool that requires us to change our coding/naming conventions (as opposed to merely how we mark stuff up in comments or whatever) is a broken tool. Doubly so if it mandates violation of the C standards. We want to be able to document the structs/whatever we have now, not only after we've done some enormous change to every struct we have to bring it into line with somebody else's conventions. -- PMM
On 12/14/2011 11:11 AM, Peter Maydell wrote: > On 14 December 2011 16:55, Anthony Liguori<anthony@codemonkey.ws> wrote: >> I think in this case, we're just going to have to deviate from the standard. >> The benefit is great, the practice is widespread, and there's no immediate >> likelihood of changing glib's coding style practices. >> >> I still think we should adhere to this policy where ever possible but I >> think we'll have to make an exception for extracting documentation. > > Disagree. A docs tool that requires us to change our coding/naming > conventions (as opposed to merely how we mark stuff up in comments > or whatever) is a broken tool. Doubly so if it mandates violation > of the C standards. > > We want to be able to document the structs/whatever we have now, > not only after we've done some enormous change to every struct we > have to bring it into line with somebody else's conventions. If you have a better suggestion, I'm all for it. But since C is not the most parseable language ever invented, basically every tool comes with constraints about coding style. We can spend forever searching for the perfect tool, invent one ourselves, or just suck it up and use what's already out there. The C language enforcement police aren't going to come knocking at our doors tomorrow. This doesn't create a bug or remove a feature. At some level, we need to be pragmatic. Regards, Anthony Liguori > > -- PMM >
On 14 December 2011 17:19, Anthony Liguori <anthony@codemonkey.ws> wrote:
> If you have a better suggestion, I'm all for it.
Shrug. I'm not sure what problem you're solving here. The bits
of QEMU that are the worst to understand are the ones which aren't
documented at all, and the bits where the general structure and
underlying assumptions aren't documented. I'm happy to look in a
header file for specific function API info.
-- PMM
Am 14.12.2011 18:19, schrieb Anthony Liguori: > On 12/14/2011 11:11 AM, Peter Maydell wrote: >> On 14 December 2011 16:55, Anthony Liguori<anthony@codemonkey.ws> >> wrote: >>> I think in this case, we're just going to have to deviate from the >>> standard. >>> The benefit is great, the practice is widespread, and there's no >>> immediate >>> likelihood of changing glib's coding style practices. >>> >>> I still think we should adhere to this policy where ever possible but I >>> think we'll have to make an exception for extracting documentation. >> >> Disagree. A docs tool that requires us to change our coding/naming >> conventions (as opposed to merely how we mark stuff up in comments >> or whatever) is a broken tool. Doubly so if it mandates violation >> of the C standards. >> >> We want to be able to document the structs/whatever we have now, >> not only after we've done some enormous change to every struct we >> have to bring it into line with somebody else's conventions. > > If you have a better suggestion, I'm all for it. But since C is not > the most parseable language ever invented, basically every tool comes > with constraints about coding style. > > We can spend forever searching for the perfect tool, invent one > ourselves, or just suck it up and use what's already out there. > > The C language enforcement police aren't going to come knocking at our > doors tomorrow. This doesn't create a bug or remove a feature. > > At some level, we need to be pragmatic. > > Regards, > > Anthony Liguori Would 's' instead of '_' work? struct sCamelCase; typedef struct sCamelCase { // ... } CamelCase; It does not violate any standard... Regards, Stefan Weil
On 12/14/2011 11:26 AM, Peter Maydell wrote: > On 14 December 2011 17:19, Anthony Liguori<anthony@codemonkey.ws> wrote: >> If you have a better suggestion, I'm all for it. > > Shrug. I'm not sure what problem you're solving here. To introduce the infrastructure such that we can have high quality internal documentation such that I can use that infrastructure with QEMU Object Model. > The bits > of QEMU that are the worst to understand are the ones which aren't > documented at all, What are the bits that aren't documented at all? An easy answer to that would be "anything that isn't present on http://wiki.qemu.org/docs-internal" How does someone help documented the undocumented things? Send a patch adding gtk-doc style documentation to a file and then once it's merged it will automagically appear on qemu.org. I think that goes a long way to solving the problem. > and the bits where the general structure and > underlying assumptions aren't documented. I'm happy to look in a > header file for specific function API info. I really prefer reading it in the form of a reference document. It also helps enforce a certain amount of consistency in the format of the documentation which is hard to do if there isn't some sort of tool running against it. Regards, Anthony Liguori > -- PMM >
Am 14.12.2011 17:34, schrieb malc: > On Wed, 14 Dec 2011, Anthony Liguori wrote: > >> GTK/glib uses a convenient of: >> >> typedef struct _CamelCase CamelCase; >> >> The reason that they use a separate struct name is that in C++, the >> struct >> namespace not a separate namespace from the type namespace. This is >> actually a >> reasonable policy for QEMU to adopt as we eventually start exporting >> C libraries >> that may be consumed by C++ programs. >> >> I think the use of _ does not violate the C specification as the struct >> namespace is not the same as the type namespace which is what the C >> spec refers >> to if I understand it correctly. > > It does violate the standard _ followed by upper case letter is reserved > in all contexts. sCamelCase instead of _CamelCase seems to work, too. I just finished a first test with gtk-doc and had no problems. So it's possible to support gtk-doc (which is a good thing) _and_ keep the standard (which is very important, too). The new rule for structure declarations in QEMU could be like this: /* forward declaration */ struct sCamelCase; /* struct definition */ typedef struct sCamelCase { /* values follow here ... */ } CamelCase; Structures which don't need a forward declaration can use simplified definitions: typedef struct { /* values follow here ... */ } CamelCaseWithoutForwardDeclaration; Regards, Stefan Weil
On 12/14/2011 12:54 PM, Stefan Weil wrote: > Am 14.12.2011 17:34, schrieb malc: >> On Wed, 14 Dec 2011, Anthony Liguori wrote: >> >>> GTK/glib uses a convenient of: >>> >>> typedef struct _CamelCase CamelCase; >>> >>> The reason that they use a separate struct name is that in C++, the struct >>> namespace not a separate namespace from the type namespace. This is actually a >>> reasonable policy for QEMU to adopt as we eventually start exporting C libraries >>> that may be consumed by C++ programs. >>> >>> I think the use of _ does not violate the C specification as the struct >>> namespace is not the same as the type namespace which is what the C spec refers >>> to if I understand it correctly. >> >> It does violate the standard _ followed by upper case letter is reserved >> in all contexts. > > sCamelCase instead of _CamelCase seems to work, too. Are you sure? Take a look at: html/QEMU-Memory-API.html#MemoryRegionOps It's supposed to look like: http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps Regards, Anthony Liguori
On 12/14/2011 11:54 AM, Stefan Weil wrote: >> It does violate the standard _ followed by upper case letter is reserved >> in all contexts. > > sCamelCase instead of _CamelCase seems to work, too. What about _camelCase instead of _CamelCase? That preserves the leading underscore, but no longer uses the reserved _ followed by a capital. At any rate, that's the convention libvirt is using; all public structs are prefixed with vir, so things like: typedef struct _virConnect virConnect; are both parseable by gtkdocs as well as C standard compliant.
On Wed, 14 Dec 2011, Eric Blake wrote: > On 12/14/2011 11:54 AM, Stefan Weil wrote: > >> It does violate the standard _ followed by upper case letter is reserved > >> in all contexts. > > > > sCamelCase instead of _CamelCase seems to work, too. > > What about _camelCase instead of _CamelCase? That preserves the leading > underscore, but no longer uses the reserved _ followed by a capital. > > At any rate, that's the convention libvirt is using; all public structs > are prefixed with vir, so things like: > > typedef struct _virConnect virConnect; > > are both parseable by gtkdocs as well as C standard compliant. > > 7.1.3 7.1.3 Reserved identifiers 1 Each header declares or defines all identifiers listed in its associated subclause, and optionally declares or defines identifiers listed in its associated future library directions subclause and identifiers which are always reserved either for any use or for use as file scope identifiers. - All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use. - All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces [..snip..] IOW - no, that's not good either.
Am 14.12.2011 20:03, schrieb Anthony Liguori: > On 12/14/2011 12:54 PM, Stefan Weil wrote: >> Am 14.12.2011 17:34, schrieb malc: >>> On Wed, 14 Dec 2011, Anthony Liguori wrote: >>> >>>> GTK/glib uses a convenient of: >>>> >>>> typedef struct _CamelCase CamelCase; >>>> >>>> The reason that they use a separate struct name is that in C++, the >>>> struct >>>> namespace not a separate namespace from the type namespace. This is >>>> actually a >>>> reasonable policy for QEMU to adopt as we eventually start >>>> exporting C libraries >>>> that may be consumed by C++ programs. >>>> >>>> I think the use of _ does not violate the C specification as the >>>> struct >>>> namespace is not the same as the type namespace which is what the C >>>> spec refers >>>> to if I understand it correctly. >>> >>> It does violate the standard _ followed by upper case letter is >>> reserved >>> in all contexts. >> >> sCamelCase instead of _CamelCase seems to work, too. > > Are you sure? > > Take a look at: > > html/QEMU-Memory-API.html#MemoryRegionOps > > It's supposed to look like: > > http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps > > Regards, > > Anthony Liguori I took a look. The html documentation claims that there is a "struct MemoryRegion". There isn't, it's a typedef. Users of the API should use a pointer to a MemoryRegion without knowing details of MemoryRegion, not even whether it is a struct, long or something else. What problems do you see from using sCamelCase? Maybe I am looking in the wrong corner. Regards, Stefan Weil
On 12/14/2011 02:23 PM, Eric Blake wrote: > On 12/14/2011 11:54 AM, Stefan Weil wrote: >>> It does violate the standard _ followed by upper case letter is reserved >>> in all contexts. >> >> sCamelCase instead of _CamelCase seems to work, too. > > What about _camelCase instead of _CamelCase? That preserves the leading > underscore, but no longer uses the reserved _ followed by a capital. > > At any rate, that's the convention libvirt is using; all public structs > are prefixed with vir, so things like: > > typedef struct _virConnect virConnect; > > are both parseable by gtkdocs as well as C standard compliant. I just hacked gtk-doc. That seems like the easiest solution to the problem. Regards, Anthony Liguori >
On 12/14/2011 02:48 PM, Stefan Weil wrote: > Am 14.12.2011 20:03, schrieb Anthony Liguori: >> On 12/14/2011 12:54 PM, Stefan Weil wrote: >>> Am 14.12.2011 17:34, schrieb malc: >>>> On Wed, 14 Dec 2011, Anthony Liguori wrote: >>>> >>>>> GTK/glib uses a convenient of: >>>>> >>>>> typedef struct _CamelCase CamelCase; >>>>> >>>>> The reason that they use a separate struct name is that in C++, the struct >>>>> namespace not a separate namespace from the type namespace. This is actually a >>>>> reasonable policy for QEMU to adopt as we eventually start exporting C >>>>> libraries >>>>> that may be consumed by C++ programs. >>>>> >>>>> I think the use of _ does not violate the C specification as the struct >>>>> namespace is not the same as the type namespace which is what the C spec >>>>> refers >>>>> to if I understand it correctly. >>>> >>>> It does violate the standard _ followed by upper case letter is reserved >>>> in all contexts. >>> >>> sCamelCase instead of _CamelCase seems to work, too. >> >> Are you sure? >> >> Take a look at: >> >> html/QEMU-Memory-API.html#MemoryRegionOps >> >> It's supposed to look like: >> >> http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps >> >> Regards, >> >> Anthony Liguori > > I took a look. The html documentation claims that there is a > "struct MemoryRegion". There isn't, it's a typedef. > Users of the API should use a pointer to a MemoryRegion > without knowing details of MemoryRegion, not even whether > it is a struct, long or something else. You should see documentation for each parameter. At this point, I just patched gtk-doc such that now gtk-doc doesn't require the underscore. At least the latest version of gtk-doc definitely requires an underscore so having a patched version will be necessary. Regards, Anthony Liguori
Am 14.12.2011 21:54, schrieb Anthony Liguori: > On 12/14/2011 02:48 PM, Stefan Weil wrote: >> Am 14.12.2011 20:03, schrieb Anthony Liguori: >>> On 12/14/2011 12:54 PM, Stefan Weil wrote: >>>> Am 14.12.2011 17:34, schrieb malc: >>>>> On Wed, 14 Dec 2011, Anthony Liguori wrote: >>>>> >>>>>> GTK/glib uses a convenient of: >>>>>> >>>>>> typedef struct _CamelCase CamelCase; >>>>>> >>>>>> The reason that they use a separate struct name is that in C++, >>>>>> the struct >>>>>> namespace not a separate namespace from the type namespace. This >>>>>> is actually a >>>>>> reasonable policy for QEMU to adopt as we eventually start >>>>>> exporting C >>>>>> libraries >>>>>> that may be consumed by C++ programs. >>>>>> >>>>>> I think the use of _ does not violate the C specification as the >>>>>> struct >>>>>> namespace is not the same as the type namespace which is what the >>>>>> C spec >>>>>> refers >>>>>> to if I understand it correctly. >>>>> >>>>> It does violate the standard _ followed by upper case letter is >>>>> reserved >>>>> in all contexts. >>>> >>>> sCamelCase instead of _CamelCase seems to work, too. >>> >>> Are you sure? >>> >>> Take a look at: >>> >>> html/QEMU-Memory-API.html#MemoryRegionOps >>> >>> It's supposed to look like: >>> >>> http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps >>> >>> Regards, >>> >>> Anthony Liguori >> >> I took a look. The html documentation claims that there is a >> "struct MemoryRegion". There isn't, it's a typedef. >> Users of the API should use a pointer to a MemoryRegion >> without knowing details of MemoryRegion, not even whether >> it is a struct, long or something else. > > You should see documentation for each parameter. > > At this point, I just patched gtk-doc such that now gtk-doc doesn't > require the underscore. > > At least the latest version of gtk-doc definitely requires an > underscore so having a patched version will be necessary. > > Regards, > > Anthony Liguori > See http://qemu.weilnetz.de/gtkdoc/QEMU-Memory-API.html for the result which I get with latest QEMU sources, your patch series v2 and gtk-doc from Debian Squeeze. Regards, Stefan Weil
On 12/14/2011 03:26 PM, Stefan Weil wrote: > Am 14.12.2011 21:54, schrieb Anthony Liguori: >> On 12/14/2011 02:48 PM, Stefan Weil wrote: >>> Am 14.12.2011 20:03, schrieb Anthony Liguori: >>>> On 12/14/2011 12:54 PM, Stefan Weil wrote: >>>>> Am 14.12.2011 17:34, schrieb malc: >>>>>> On Wed, 14 Dec 2011, Anthony Liguori wrote: >>>>>> >>>>>>> GTK/glib uses a convenient of: >>>>>>> >>>>>>> typedef struct _CamelCase CamelCase; >>>>>>> >>>>>>> The reason that they use a separate struct name is that in C++, the struct >>>>>>> namespace not a separate namespace from the type namespace. This is >>>>>>> actually a >>>>>>> reasonable policy for QEMU to adopt as we eventually start exporting C >>>>>>> libraries >>>>>>> that may be consumed by C++ programs. >>>>>>> >>>>>>> I think the use of _ does not violate the C specification as the struct >>>>>>> namespace is not the same as the type namespace which is what the C spec >>>>>>> refers >>>>>>> to if I understand it correctly. >>>>>> >>>>>> It does violate the standard _ followed by upper case letter is reserved >>>>>> in all contexts. >>>>> >>>>> sCamelCase instead of _CamelCase seems to work, too. >>>> >>>> Are you sure? >>>> >>>> Take a look at: >>>> >>>> html/QEMU-Memory-API.html#MemoryRegionOps >>>> >>>> It's supposed to look like: >>>> >>>> http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps >>>> >>>> Regards, >>>> >>>> Anthony Liguori >>> >>> I took a look. The html documentation claims that there is a >>> "struct MemoryRegion". There isn't, it's a typedef. >>> Users of the API should use a pointer to a MemoryRegion >>> without knowing details of MemoryRegion, not even whether >>> it is a struct, long or something else. >> >> You should see documentation for each parameter. >> >> At this point, I just patched gtk-doc such that now gtk-doc doesn't require >> the underscore. >> >> At least the latest version of gtk-doc definitely requires an underscore so >> having a patched version will be necessary. >> >> Regards, >> >> Anthony Liguori >> > > > See http://qemu.weilnetz.de/gtkdoc/QEMU-Memory-API.html for > the result which I get with latest QEMU sources, your patch > series v2 and gtk-doc from Debian Squeeze. Look carefully at: http://qemu.weilnetz.de/gtkdoc/QEMU-Memory-API.html#MemoryRegionOps vs: http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps There's a significant difference :-) Regards, Anthony Liguori > > Regards, > Stefan Weil > >
Am 14.12.2011 22:51, schrieb Anthony Liguori: > > Look carefully at: > > http://qemu.weilnetz.de/gtkdoc/QEMU-Memory-API.html#MemoryRegionOps > > vs: > > http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps > > There's a significant difference :-) > > Regards, > > Anthony Liguori I tried the following declaration: typedef struct sMemoryRegionOps { uint64_t (*read)(void *opaque, target_phys_addr_t addr, unsigned size); void (*write)(void *opaque, target_phys_addr_t addr, uint64_t data, unsigned size); enum device_endian endianness; MemoryRegionGuestConstraints valid; MemoryRegionInternalConstraints impl; const MemoryRegionPortio *old_portio; const MemoryRegionMmio old_mmio; } MemoryRegionOps; See the result here: http://qemu.weilnetz.de/gtkdoc4/QEMU-Memory-API.html#MemoryRegionOps Regards, Stefan Weil
On 12/14/2011 04:23 PM, Stefan Weil wrote: > Am 14.12.2011 22:51, schrieb Anthony Liguori: >> >> Look carefully at: >> >> http://qemu.weilnetz.de/gtkdoc/QEMU-Memory-API.html#MemoryRegionOps >> >> vs: >> >> http://wiki.qemu.org/docs-internal/QEMU-Memory-API.html#MemoryRegionOps >> >> There's a significant difference :-) >> >> Regards, >> >> Anthony Liguori > > I tried the following declaration: > > typedef struct sMemoryRegionOps { > uint64_t (*read)(void *opaque, > target_phys_addr_t addr, > unsigned size); > void (*write)(void *opaque, > target_phys_addr_t addr, > uint64_t data, > unsigned size); > > enum device_endian endianness; > > MemoryRegionGuestConstraints valid; > MemoryRegionInternalConstraints impl; > > const MemoryRegionPortio *old_portio; > const MemoryRegionMmio old_mmio; > } MemoryRegionOps; > > See the result here: > http://qemu.weilnetz.de/gtkdoc4/QEMU-Memory-API.html#MemoryRegionOps Interesting, but it wouldn't be possible to do a forward declaration? I think my patch to gtk-doc (make _ optional) seems reasonable and I think that's a bit nicer than doing struct sCamelCase too. That doesn't help with C++ compatibility but now that it is not in favor of my argument, I no longer care about it ;-) (j/k) Regards, Anthony Liguori > Regards, > Stefan Weil > >
On 12/14/2011 07:55 PM, Stefan Weil wrote: > > > Would 's' instead of '_' work? > > struct sCamelCase; > > typedef struct sCamelCase { > // ... > } CamelCase; > > It does not violate any standard... Would '' instead of 's' work? typedes struct CamelCase { ... } CamelCase;
On 12/15/2011 03:20 AM, Avi Kivity wrote: > On 12/14/2011 07:55 PM, Stefan Weil wrote: >> >> >> Would 's' instead of '_' work? >> >> struct sCamelCase; >> >> typedef struct sCamelCase { >> // ... >> } CamelCase; >> >> It does not violate any standard... > > Would '' instead of 's' work? > > typedes struct CamelCase { > ... > } CamelCase; Not in upstream gtk-doc. The '_' is explicitly looked for. Regards, Anthony Liguori >
diff --git a/ioport.h b/ioport.h index ae3e9da..99345d8 100644 --- a/ioport.h +++ b/ioport.h @@ -52,24 +52,24 @@ uint8_t cpu_inb(pio_addr_t addr); uint16_t cpu_inw(pio_addr_t addr); uint32_t cpu_inl(pio_addr_t addr); -struct MemoryRegion; -struct MemoryRegionPortio; +struct _MemoryRegion; +struct _MemoryRegionPortio; typedef struct PortioList { - const struct MemoryRegionPortio *ports; - struct MemoryRegion *address_space; + const struct _MemoryRegionPortio *ports; + struct _MemoryRegion *address_space; unsigned nr; - struct MemoryRegion **regions; + struct _MemoryRegion **regions; void *opaque; const char *name; } PortioList; void portio_list_init(PortioList *piolist, - const struct MemoryRegionPortio *callbacks, + const struct _MemoryRegionPortio *callbacks, void *opaque, const char *name); void portio_list_destroy(PortioList *piolist); void portio_list_add(PortioList *piolist, - struct MemoryRegion *address_space, + struct _MemoryRegion *address_space, uint32_t addr); void portio_list_del(PortioList *piolist); diff --git a/memory.c b/memory.c index adfdf14..76a7ae6 100644 --- a/memory.c +++ b/memory.c @@ -72,12 +72,12 @@ static AddrRange addrrange_intersection(AddrRange r1, AddrRange r2) return addrrange_make(start, int128_sub(end, start)); } -struct CoalescedMemoryRange { +struct _CoalescedMemoryRange { AddrRange addr; - QTAILQ_ENTRY(CoalescedMemoryRange) link; + QTAILQ_ENTRY(_CoalescedMemoryRange) link; }; -struct MemoryRegionIoeventfd { +struct _MemoryRegionIoeventfd { AddrRange addr; bool match_data; uint64_t data; diff --git a/memory.h b/memory.h index beae127..3aa8404 100644 --- a/memory.h +++ b/memory.h @@ -26,10 +26,12 @@ #include "ioport.h" #include "int128.h" -typedef struct MemoryRegionOps MemoryRegionOps; -typedef struct MemoryRegion MemoryRegion; -typedef struct MemoryRegionPortio MemoryRegionPortio; -typedef struct MemoryRegionMmio MemoryRegionMmio; +typedef struct _MemoryRegionOps MemoryRegionOps; +typedef struct _MemoryRegion MemoryRegion; +typedef struct _MemoryRegionPortio MemoryRegionPortio; +typedef struct _MemoryRegionMmio MemoryRegionMmio; +typedef struct _MemoryRegionGuestConstraints MemoryRegionGuestConstraints; +typedef struct _MemoryRegionInternalConstraints MemoryRegionInternalConstraints; /* Must match *_DIRTY_FLAGS in cpu-all.h. To be replaced with dynamic * registration. @@ -38,15 +40,51 @@ typedef struct MemoryRegionMmio MemoryRegionMmio; #define DIRTY_MEMORY_CODE 1 #define DIRTY_MEMORY_MIGRATION 3 -struct MemoryRegionMmio { +struct _MemoryRegionMmio { CPUReadMemoryFunc *read[3]; CPUWriteMemoryFunc *write[3]; }; +struct _MemoryRegionGuestConstraints +{ + /* If nonzero, specify bounds on access sizes beyond which a machine + * check is thrown. + */ + unsigned min_access_size; + unsigned max_access_size; + /* If true, unaligned accesses are supported. Otherwise unaligned + * accesses throw machine checks. + */ + bool unaligned; + /* + * If present, and returns #false, the transaction is not accepted + * by the device (and results in machine dependent behaviour such + * as a machine check exception). + */ + bool (*accepts)(void *opaque, target_phys_addr_t addr, + unsigned size, bool is_write); +}; + +struct _MemoryRegionInternalConstraints +{ + /* If nonzero, specifies the minimum size implemented. Smaller sizes + * will be rounded upwards and a partial result will be returned. + */ + unsigned min_access_size; + /* If nonzero, specifies the maximum size implemented. Larger sizes + * will be done as a series of accesses with smaller sizes. + */ + unsigned max_access_size; + /* If true, unaligned accesses are supported. Otherwise all accesses + * are converted to (possibly multiple) naturally aligned accesses. + */ + bool unaligned; +}; + /* * Memory region callbacks */ -struct MemoryRegionOps { +struct _MemoryRegionOps { /* Read from the memory region. @addr is relative to @mr; @size is * in bytes. */ uint64_t (*read)(void *opaque, @@ -61,39 +99,10 @@ struct MemoryRegionOps { enum device_endian endianness; /* Guest-visible constraints: */ - struct { - /* If nonzero, specify bounds on access sizes beyond which a machine - * check is thrown. - */ - unsigned min_access_size; - unsigned max_access_size; - /* If true, unaligned accesses are supported. Otherwise unaligned - * accesses throw machine checks. - */ - bool unaligned; - /* - * If present, and returns #false, the transaction is not accepted - * by the device (and results in machine dependent behaviour such - * as a machine check exception). - */ - bool (*accepts)(void *opaque, target_phys_addr_t addr, - unsigned size, bool is_write); - } valid; + MemoryRegionGuestConstraints valid; + /* Internal implementation constraints: */ - struct { - /* If nonzero, specifies the minimum size implemented. Smaller sizes - * will be rounded upwards and a partial result will be returned. - */ - unsigned min_access_size; - /* If nonzero, specifies the maximum size implemented. Larger sizes - * will be done as a series of accesses with smaller sizes. - */ - unsigned max_access_size; - /* If true, unaligned accesses are supported. Otherwise all accesses - * are converted to (possibly multiple) naturally aligned accesses. - */ - bool unaligned; - } impl; + MemoryRegionInternalConstraints impl; /* If .read and .write are not present, old_portio may be used for * backwards compatibility with old portio registration @@ -105,10 +114,10 @@ struct MemoryRegionOps { const MemoryRegionMmio old_mmio; }; -typedef struct CoalescedMemoryRange CoalescedMemoryRange; -typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd; +typedef struct _CoalescedMemoryRange CoalescedMemoryRange; +typedef struct _MemoryRegionIoeventfd MemoryRegionIoeventfd; -struct MemoryRegion { +struct _MemoryRegion { /* All fields are private - violators will be prosecuted */ const MemoryRegionOps *ops; void *opaque; @@ -127,16 +136,16 @@ struct MemoryRegion { target_phys_addr_t alias_offset; unsigned priority; bool may_overlap; - QTAILQ_HEAD(subregions, MemoryRegion) subregions; - QTAILQ_ENTRY(MemoryRegion) subregions_link; - QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced; + QTAILQ_HEAD(subregions, _MemoryRegion) subregions; + QTAILQ_ENTRY(_MemoryRegion) subregions_link; + QTAILQ_HEAD(coalesced_ranges, _CoalescedMemoryRange) coalesced; const char *name; uint8_t dirty_log_mask; unsigned ioeventfd_nb; MemoryRegionIoeventfd *ioeventfds; }; -struct MemoryRegionPortio { +struct _MemoryRegionPortio { uint32_t offset; uint32_t len; unsigned size;
GTK/glib uses a convenient of: typedef struct _CamelCase CamelCase; The reason that they use a separate struct name is that in C++, the struct namespace not a separate namespace from the type namespace. This is actually a reasonable policy for QEMU to adopt as we eventually start exporting C libraries that may be consumed by C++ programs. I think the use of _ does not violate the C specification as the struct namespace is not the same as the type namespace which is what the C spec refers to if I understand it correctly. Additionally, gtkdoc-scan cannot handle nested structs so remove those from the memory API. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- ioport.h | 14 ++++---- memory.c | 6 ++-- memory.h | 99 +++++++++++++++++++++++++++++++++---------------------------- 3 files changed, 64 insertions(+), 55 deletions(-)