| Message ID | AANLkTikU6F1fuY69WtT0n0whs5fkh-zv3Dpj+2rznJHd@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 12, 2010 at 5:50 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > Add memory management rules, somewhat like libvirt HACKING. > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > CODING_STYLE | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/CODING_STYLE b/CODING_STYLE > index 3f10d72..085c86f 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -148,3 +148,11 @@ up-front that this is a read-only pointer. Perhaps more > importantly, if we're diligent about this, when you see a non-const > pointer, you're guaranteed that it is used to modify the storage > it points to, or it is aliased to another pointer that is. > + > + > +7. Low level memory management > + > +Use of the malloc/free/realloc/calloc APIs is not allowed in the QEMU I forgot to add valloc/memalign/posix_memalign. > +codebase. Instead of these routines, use the replacement > +qemu_malloc/qemu_mallocz/qemu_realloc/qemu_free or > +qemu_vmalloc/qemu_memalign/qemu_vfree APIs. This should also mention that memory allocated by qemu_vmalloc or qemu_memalign should be freed with qemu_vfree. In general, whole of 7 is an existing rule and current codebase seems to follow it. This should be added as a strict rule (as opposed to guideline), since breaking this will cause problems on Win32 and user emulators.
On Fri, 13 Aug 2010, Blue Swirl wrote: > On Thu, Aug 12, 2010 at 5:50 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > > Add memory management rules, somewhat like libvirt HACKING. > > > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > > --- > > CODING_STYLE | 8 ++++++++ > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/CODING_STYLE b/CODING_STYLE > > index 3f10d72..085c86f 100644 > > --- a/CODING_STYLE > > +++ b/CODING_STYLE > > @@ -148,3 +148,11 @@ up-front that this is a read-only pointer. Perhaps more > > importantly, if we're diligent about this, when you see a non-const > > pointer, you're guaranteed that it is used to modify the storage > > it points to, or it is aliased to another pointer that is. > > + > > + > > +7. Low level memory management > > + > > +Use of the malloc/free/realloc/calloc APIs is not allowed in the QEMU > > I forgot to add valloc/memalign/posix_memalign. > > > +codebase. Instead of these routines, use the replacement > > +qemu_malloc/qemu_mallocz/qemu_realloc/qemu_free or > > +qemu_vmalloc/qemu_memalign/qemu_vfree APIs. > > This should also mention that memory allocated by qemu_vmalloc or > qemu_memalign should be freed with qemu_vfree. > > In general, whole of 7 is an existing rule and current codebase seems > to follow it. > > This should be added as a strict rule (as opposed to guideline), since > breaking this will cause problems on Win32 and user emulators. > This is all perfectly reasonable, but begs one question, where do we stop codifying self-evident truths (or who is the ultimiate judge of what is self-evident and what isn't), since taking the above to extreme we will end up adding stuff like: you must use close to dispose of open-ed descriptors and such like.
On Fri, Aug 13, 2010 at 9:01 PM, malc <av1474@comtv.ru> wrote: > On Fri, 13 Aug 2010, Blue Swirl wrote: > >> On Thu, Aug 12, 2010 at 5:50 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >> > Add memory management rules, somewhat like libvirt HACKING. >> > >> > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> > --- >> > CODING_STYLE | 8 ++++++++ >> > 1 files changed, 8 insertions(+), 0 deletions(-) >> > >> > diff --git a/CODING_STYLE b/CODING_STYLE >> > index 3f10d72..085c86f 100644 >> > --- a/CODING_STYLE >> > +++ b/CODING_STYLE >> > @@ -148,3 +148,11 @@ up-front that this is a read-only pointer. Perhaps more >> > importantly, if we're diligent about this, when you see a non-const >> > pointer, you're guaranteed that it is used to modify the storage >> > it points to, or it is aliased to another pointer that is. >> > + >> > + >> > +7. Low level memory management >> > + >> > +Use of the malloc/free/realloc/calloc APIs is not allowed in the QEMU >> >> I forgot to add valloc/memalign/posix_memalign. >> >> > +codebase. Instead of these routines, use the replacement >> > +qemu_malloc/qemu_mallocz/qemu_realloc/qemu_free or >> > +qemu_vmalloc/qemu_memalign/qemu_vfree APIs. >> >> This should also mention that memory allocated by qemu_vmalloc or >> qemu_memalign should be freed with qemu_vfree. >> >> In general, whole of 7 is an existing rule and current codebase seems >> to follow it. >> >> This should be added as a strict rule (as opposed to guideline), since >> breaking this will cause problems on Win32 and user emulators. >> > > This is all perfectly reasonable, but begs one question, where do we stop > codifying self-evident truths (or who is the ultimiate judge of what is > self-evident and what isn't), since taking the above to extreme we will > end up adding stuff like: you must use close to dispose of open-ed > descriptors and such like. Interesting question. We could assume that the target audience knows well how to write portable C, conformal to various standards, so we only have to point out QEMU specific pitfalls. This approach would probably mean that there would be no need to mention, for example, the issues with identifiers starting with an underscore. Another approach is to consider the history. Which types of problems there have been frequently with the proposed patches? Then the underscore issue (just as an example) should definitely be raised, even though that is just one aspect of standards conformance. I don't think either approach would solve your question though.
On Fri, 13 Aug 2010, Blue Swirl wrote: > On Fri, Aug 13, 2010 at 9:01 PM, malc <av1474@comtv.ru> wrote: > > On Fri, 13 Aug 2010, Blue Swirl wrote: > > > >> On Thu, Aug 12, 2010 at 5:50 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > >> > Add memory management rules, somewhat like libvirt HACKING. > >> > > >> > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > >> > --- > >> > CODING_STYLE | 8 ++++++++ > >> > 1 files changed, 8 insertions(+), 0 deletions(-) > >> > > >> > diff --git a/CODING_STYLE b/CODING_STYLE > >> > index 3f10d72..085c86f 100644 > >> > --- a/CODING_STYLE > >> > +++ b/CODING_STYLE > >> > @@ -148,3 +148,11 @@ up-front that this is a read-only pointer. Perhaps more > >> > importantly, if we're diligent about this, when you see a non-const > >> > pointer, you're guaranteed that it is used to modify the storage > >> > it points to, or it is aliased to another pointer that is. > >> > + > >> > + > >> > +7. Low level memory management > >> > + > >> > +Use of the malloc/free/realloc/calloc APIs is not allowed in the QEMU > >> > >> I forgot to add valloc/memalign/posix_memalign. > >> > >> > +codebase. Instead of these routines, use the replacement > >> > +qemu_malloc/qemu_mallocz/qemu_realloc/qemu_free or > >> > +qemu_vmalloc/qemu_memalign/qemu_vfree APIs. > >> > >> This should also mention that memory allocated by qemu_vmalloc or > >> qemu_memalign should be freed with qemu_vfree. > >> > >> In general, whole of 7 is an existing rule and current codebase seems > >> to follow it. > >> > >> This should be added as a strict rule (as opposed to guideline), since > >> breaking this will cause problems on Win32 and user emulators. > >> > > > > This is all perfectly reasonable, but begs one question, where do we stop > > codifying self-evident truths (or who is the ultimiate judge of what is > > self-evident and what isn't), since taking the above to extreme we will > > end up adding stuff like: you must use close to dispose of open-ed > > descriptors and such like. > > Interesting question. We could assume that the target audience knows > well how to write portable C, conformal to various standards, so we > only have to point out QEMU specific pitfalls. This approach would > probably mean that there would be no need to mention, for example, the > issues with identifiers starting with an underscore. > > Another approach is to consider the history. Which types of problems > there have been frequently with the proposed patches? Then the > underscore issue (just as an example) should definitely be raised, > even though that is just one aspect of standards conformance. I.e. having a (N)FAQ(BNI) [not] [but nevertheless interesting] > I don't think either approach would solve your question though. > In any case, what i wanted to convey, but failed, is that this doesn't, in my opinion, belong to the CODING_STYLE.
On Fri, Aug 13, 2010 at 9:56 PM, malc <av1474@comtv.ru> wrote: > On Fri, 13 Aug 2010, Blue Swirl wrote: > >> On Fri, Aug 13, 2010 at 9:01 PM, malc <av1474@comtv.ru> wrote: >> > On Fri, 13 Aug 2010, Blue Swirl wrote: >> > >> >> On Thu, Aug 12, 2010 at 5:50 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >> >> > Add memory management rules, somewhat like libvirt HACKING. >> >> > >> >> > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> >> > --- >> >> > CODING_STYLE | 8 ++++++++ >> >> > 1 files changed, 8 insertions(+), 0 deletions(-) >> >> > >> >> > diff --git a/CODING_STYLE b/CODING_STYLE >> >> > index 3f10d72..085c86f 100644 >> >> > --- a/CODING_STYLE >> >> > +++ b/CODING_STYLE >> >> > @@ -148,3 +148,11 @@ up-front that this is a read-only pointer. Perhaps more >> >> > importantly, if we're diligent about this, when you see a non-const >> >> > pointer, you're guaranteed that it is used to modify the storage >> >> > it points to, or it is aliased to another pointer that is. >> >> > + >> >> > + >> >> > +7. Low level memory management >> >> > + >> >> > +Use of the malloc/free/realloc/calloc APIs is not allowed in the QEMU >> >> >> >> I forgot to add valloc/memalign/posix_memalign. >> >> >> >> > +codebase. Instead of these routines, use the replacement >> >> > +qemu_malloc/qemu_mallocz/qemu_realloc/qemu_free or >> >> > +qemu_vmalloc/qemu_memalign/qemu_vfree APIs. >> >> >> >> This should also mention that memory allocated by qemu_vmalloc or >> >> qemu_memalign should be freed with qemu_vfree. >> >> >> >> In general, whole of 7 is an existing rule and current codebase seems >> >> to follow it. >> >> >> >> This should be added as a strict rule (as opposed to guideline), since >> >> breaking this will cause problems on Win32 and user emulators. >> >> >> > >> > This is all perfectly reasonable, but begs one question, where do we stop >> > codifying self-evident truths (or who is the ultimiate judge of what is >> > self-evident and what isn't), since taking the above to extreme we will >> > end up adding stuff like: you must use close to dispose of open-ed >> > descriptors and such like. >> >> Interesting question. We could assume that the target audience knows >> well how to write portable C, conformal to various standards, so we >> only have to point out QEMU specific pitfalls. This approach would >> probably mean that there would be no need to mention, for example, the >> issues with identifiers starting with an underscore. >> >> Another approach is to consider the history. Which types of problems >> there have been frequently with the proposed patches? Then the >> underscore issue (just as an example) should definitely be raised, >> even though that is just one aspect of standards conformance. > > I.e. having a (N)FAQ(BNI) > > [not] [but nevertheless interesting] > >> I don't think either approach would solve your question though. >> > > In any case, what i wanted to convey, but failed, is that this > doesn't, in my opinion, belong to the CODING_STYLE. Well, this is not coding style but architectural guide. We could introduce a new document, rename CODING_STYLE or bury this to qemu-tech.texi. But I still (optimistically) think that some kind of READ_ME_BEFORE_CODING would help.
Am 14.08.2010 um 12:02 schrieb Blue Swirl: > On Fri, Aug 13, 2010 at 9:56 PM, malc <av1474@comtv.ru> wrote: >> In any case, what i wanted to convey, but failed, is that this >> doesn't, in my opinion, belong to the CODING_STYLE. > > Well, this is not coding style but architectural guide. We could > introduce a new document, rename CODING_STYLE or bury this to > qemu-tech.texi. But I still (optimistically) think that some kind of > READ_ME_BEFORE_CODING would help. I agree that we shouldn't overload CODING_STYLE. qemu-tech.texi historically was more a description of how QEMU works. Is there a reason not to simply put this FAQ/guide into the new Wiki? Otherwise we could stick with HACKING as file name. Andreas
On 08/14/2010 10:52 AM, Andreas Färber wrote: >> >> Well, this is not coding style but architectural guide. We could >> introduce a new document, rename CODING_STYLE or bury this to >> qemu-tech.texi. But I still (optimistically) think that some kind of >> READ_ME_BEFORE_CODING would help. > > I agree that we shouldn't overload CODING_STYLE. > > qemu-tech.texi historically was more a description of how QEMU works. > Is there a reason not to simply put this FAQ/guide into the new Wiki? > Otherwise we could stick with HACKING as file name. If HACKING is chosen, it would anyway contain CODING_STYLE. I don't have any preference as long as the content is kept in only one file. Actually, I have a preference for CODING_STYLE since I see no reason to rename a file (also considering git's rename tracking is not perfect). Paolo
On 08/14/2010 12:01 AM, malc wrote: > > This is all perfectly reasonable, but begs one question, where do we stop > codifying self-evident truths (or who is the ultimiate judge of what is > self-evident and what isn't), since taking the above to extreme we will > end up adding stuff like: you must use close to dispose of open-ed > descriptors and such like. It's a matter of pragmatism, if a lot of patches are routinely rejected due to some error then it makes sense to add it to CODING_STYLE, even if they are not exactly style decisions. If not then they just dilute that document.
diff --git a/CODING_STYLE b/CODING_STYLE index 3f10d72..085c86f 100644 --- a/CODING_STYLE +++ b/CODING_STYLE @@ -148,3 +148,11 @@ up-front that this is a read-only pointer. Perhaps more importantly, if we're diligent about this, when you see a non-const pointer, you're guaranteed that it is used to modify the storage it points to, or it is aliased to another pointer that is. + + +7. Low level memory management + +Use of the malloc/free/realloc/calloc APIs is not allowed in the QEMU +codebase. Instead of these routines, use the replacement +qemu_malloc/qemu_mallocz/qemu_realloc/qemu_free or +qemu_vmalloc/qemu_memalign/qemu_vfree APIs.
Add memory management rules, somewhat like libvirt HACKING. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- CODING_STYLE | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)