[Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[zwu.kernel]

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)
 {
-- 
1.7.6

Re: [Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[Michael S. Tsirkin]

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

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/2] slirp: remove duplicate definition

[Jan Kiszka]

On 2012-02-15 09:30, 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.

I've pushed the following into queues/slirp now. Michael, please use it
as base for your renaming.

-----8<-----

slirp: Clean up ifs_init

Remove duplicate ifs_init macros, reimplement the logic as static inline
in mbuf.h.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c   |    2 --
 slirp/if.h   |    2 --
 slirp/mbuf.h |    5 +++++
 3 files changed, 5 insertions(+), 4 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)
 {
diff --git a/slirp/if.h b/slirp/if.h
index 2dac1c7..3327023 100644
--- a/slirp/if.h
+++ b/slirp/if.h
@@ -20,6 +20,4 @@
 /* 2 for alignment, 14 for ethernet, 40 for TCP/IP */
 #define IF_MAXLINKHDR (2 + 14 + 40)
 
-#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
-
 #endif
diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index 0708840..8d7951f 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -124,4 +124,9 @@ void m_adj(struct mbuf *, int);
 int m_copy(struct mbuf *, struct mbuf *, int, int);
 struct mbuf * dtom(Slirp *, void *);
 
+static inline void ifs_init(struct mbuf *ifm)
+{
+    ifm->ifs_next = ifm->ifs_prev = ifm;
+}
+
 #endif