Message ID | 1329293521-16197-1-git-send-email-zwu.kernel@gmail.com |
---|---|
State | New |
Headers | show |
On 2012-02-15 09:12, zwu.kernel@gmail.com wrote: > From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > --- > slirp/if.c | 2 -- > 1 files changed, 0 insertions(+), 2 deletions(-) > > diff --git a/slirp/if.c b/slirp/if.c > index 2852396..8e0cac2 100644 > --- a/slirp/if.c > +++ b/slirp/if.c > @@ -8,8 +8,6 @@ > #include <slirp.h> > #include "qemu-timer.h" > > -#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm)) > - > static void > ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead) > { Let's grab the chance and move ifs_init to mbuf.h. Jan
On Wed, Feb 15, 2012 at 09:30:23AM +0100, Jan Kiszka wrote: > On 2012-02-15 09:12, zwu.kernel@gmail.com wrote: > > From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > > > Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > --- > > slirp/if.c | 2 -- > > 1 files changed, 0 insertions(+), 2 deletions(-) > > > > diff --git a/slirp/if.c b/slirp/if.c > > index 2852396..8e0cac2 100644 > > --- a/slirp/if.c > > +++ b/slirp/if.c > > @@ -8,8 +8,6 @@ > > #include <slirp.h> > > #include "qemu-timer.h" > > > > -#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm)) > > - > > static void > > ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead) > > { > > Let's grab the chance and move ifs_init to mbuf.h. > > Jan > Since you mention it - why does slirp have all these defines in the first place? slirp/mbuf.h:#define m_nextpkt m_hdr.mh_nextpkt slirp/mbuf.h:#define ifs_next m_nextpkt Seriously, #define for a field name? This is just crazy, and violates our coding style which requires macros to be PPER_CAS_WITH_UNDERSCORES
On 2012-02-15 10:38, Michael S. Tsirkin wrote: > On Wed, Feb 15, 2012 at 09:30:23AM +0100, Jan Kiszka wrote: >> On 2012-02-15 09:12, zwu.kernel@gmail.com wrote: >>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> >>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> --- >>> slirp/if.c | 2 -- >>> 1 files changed, 0 insertions(+), 2 deletions(-) >>> >>> diff --git a/slirp/if.c b/slirp/if.c >>> index 2852396..8e0cac2 100644 >>> --- a/slirp/if.c >>> +++ b/slirp/if.c >>> @@ -8,8 +8,6 @@ >>> #include <slirp.h> >>> #include "qemu-timer.h" >>> >>> -#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm)) >>> - >>> static void >>> ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead) >>> { >> >> Let's grab the chance and move ifs_init to mbuf.h. >> >> Jan >> > > Since you mention it - why does slirp have all these > defines in the first place? > > slirp/mbuf.h:#define m_nextpkt m_hdr.mh_nextpkt > slirp/mbuf.h:#define ifs_next m_nextpkt > > Seriously, #define for a field name? > This is just crazy, and violates our coding style > which requires macros to be PPER_CAS_WITH_UNDERSCORES > This is a total mess, I agree, and I'd warmly welcome patches to clean this up. I can only speculate on the reasons. Maybe the intention was to keep the core minimally modified and wrap all the conflicts it causes away. Given that slirp outside QEMU is dead, this is no longer a valid reason (if it ever was). Jan
Am 15.02.2012 10:38, schrieb Michael S. Tsirkin: > why does slirp have all these > defines in the first place? > > slirp/mbuf.h:#define m_nextpkt m_hdr.mh_nextpkt > slirp/mbuf.h:#define ifs_next m_nextpkt > > Seriously, #define for a field name? We have some such #defines in ibm8514 graphics card code, too (not yet upstream). It is handy to make code match the datasheet while hiding some of the implementation details, like two levels of array accesses into implementation-defined-or-whatever registers. Doesn't seem to apply here obviously. Maybe historically m_hdr.mh_nextpkt was m_nextpkt and someone tried to avoid a full refactoring. Andreas
On 15 February 2012 23:02, Andreas Färber <afaerber@suse.de> wrote: > Am 15.02.2012 10:38, schrieb Michael S. Tsirkin: >> why does slirp have all these >> defines in the first place? >> >> slirp/mbuf.h:#define m_nextpkt m_hdr.mh_nextpkt >> slirp/mbuf.h:#define ifs_next m_nextpkt >> >> Seriously, #define for a field name? > Maybe historically > m_hdr.mh_nextpkt was m_nextpkt and someone tried to avoid a full > refactoring. This comes from a BSD header originally, and I guess in the BSD context it would have been preserving source compatibility with older versions of BSD where the fields really were all in the mbuf struct. Here's 4.2BSD, for instance: http://minnie.tuhs.org/cgi-bin/utree.pl?file=4.2BSD/usr/src/sys/h/mbuf.h The split out of the header fields and the introduction of these macros seems to have happened somewhere between 4.3BSD-Tahoe in 1988 and 4.3BSD-Reno in 1990: you can see them here: http://minnie.tuhs.org/cgi-bin/utree.pl?file=4.3BSD-Reno/src/sys/sys/mbuf.h Anyway, we can happily clean up since we're not trying to be backward compatible with anything (assuming you have no objection to deleting a historical artefact that's over twenty years old :-)) -- PMM
diff --git a/slirp/if.c b/slirp/if.c index 2852396..8e0cac2 100644 --- a/slirp/if.c +++ b/slirp/if.c @@ -8,8 +8,6 @@ #include <slirp.h> #include "qemu-timer.h" -#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm)) - static void ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead) {