[1/2] slirp: remove duplicate definition

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(-)

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