[Qemu-devel] [PATCH 0/2] slirp: remove fd_block/fd_nonblock

[Qemu-devel] [PATCH 0/2] slirp: remove fd_block/fd_nonblock

[Paolo Bonzini]

Of these two functions, one is unused and the other is a dup of
socket_set_nonblock.  Unfortunately, there are some conflicts
with system headers that need to be sorted out first.

Paolo Bonzini (2):
  slirp: clean up conflicts with system headers
  slirp: use socket_set_nonblock

 slirp/misc.c     |   46 +---------------------------------------------
 slirp/slirp.h    |    8 +-------
 slirp/tcp.h      |   21 +++++++++++++++------
 slirp/tcp_subr.c |    4 ++--
 4 files changed, 19 insertions(+), 60 deletions(-)

-- 
1.7.9.1

[Qemu-devel] [PATCH 1/2] slirp: clean up conflicts with system headers

[Paolo Bonzini]

Right now, slirp/slirp.h cannot include some system headers and,
indirectly, qemu_socket.h.  Clean this up, and remove a duplicate
prototype that was introduced because of that.


Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 slirp/slirp.h |    8 +-------
 slirp/tcp.h   |   21 +++++++++++++++------
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/slirp/slirp.h b/slirp/slirp.h
index 5033ee3..2098b20 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -88,10 +88,6 @@ void *malloc(size_t arg);
 void free(void *ptr);
 #endif
 
-#ifndef HAVE_INET_ATON
-int inet_aton(const char *cp, struct in_addr *ia);
-#endif
-
 #include <fcntl.h>
 #ifndef NO_UNIX_SOCKETS
 #include <sys/un.h>
@@ -144,6 +140,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "debug.h"
 
 #include "qemu-queue.h"
+#include "qemu_socket.h"
 
 #include "libslirp.h"
 #include "ip.h"
@@ -167,9 +164,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "bootp.h"
 #include "tftp.h"
 
-/* osdep.c */
-int qemu_socket(int domain, int type, int protocol);
-
 #define ETH_ALEN 6
 #define ETH_HLEN 14
 
diff --git a/slirp/tcp.h b/slirp/tcp.h
index b3817cb..8299603 100644
--- a/slirp/tcp.h
+++ b/slirp/tcp.h
@@ -45,6 +45,7 @@ typedef	uint32_t tcp_seq;
  * TCP header.
  * Per RFC 793, September, 1981.
  */
+#define tcphdr slirp_tcphdr
 struct tcphdr {
 	uint16_t th_sport;              /* source port */
 	uint16_t th_dport;              /* destination port */
@@ -58,12 +59,6 @@ struct tcphdr {
 		th_off:4;		/* data offset */
 #endif
 	uint8_t th_flags;
-#define	TH_FIN	0x01
-#define	TH_SYN	0x02
-#define	TH_RST	0x04
-#define	TH_PUSH	0x08
-#define	TH_ACK	0x10
-#define	TH_URG	0x20
 	uint16_t th_win;                /* window */
 	uint16_t th_sum;                /* checksum */
 	uint16_t th_urp;                /* urgent pointer */
@@ -71,6 +66,16 @@ struct tcphdr {
 
 #include "tcp_var.h"
 
+#ifndef TH_FIN
+#define	TH_FIN	0x01
+#define	TH_SYN	0x02
+#define	TH_RST	0x04
+#define	TH_PUSH	0x08
+#define	TH_ACK	0x10
+#define	TH_URG	0x20
+#endif
+
+#ifndef TCPOPT_EOL
 #define	TCPOPT_EOL		0
 #define	TCPOPT_NOP		1
 #define	TCPOPT_MAXSEG		2
@@ -86,6 +91,7 @@ struct tcphdr {
 
 #define TCPOPT_TSTAMP_HDR	\
     (TCPOPT_NOP<<24|TCPOPT_NOP<<16|TCPOPT_TIMESTAMP<<8|TCPOLEN_TIMESTAMP)
+#endif
 
 /*
  * Default maximum segment size for TCP.
@@ -95,10 +101,13 @@ struct tcphdr {
  *
  * We make this 1460 because we only care about Ethernet in the qemu context.
  */
+#undef TCP_MSS
 #define	TCP_MSS	1460
 
+#undef TCP_MAXWIN
 #define	TCP_MAXWIN	65535	/* largest value for (unscaled) window */
 
+#undef TCP_MAX_WINSHIFT
 #define TCP_MAX_WINSHIFT	14	/* maximum window shift */
 
 /*
-- 
1.7.9.1

[Qemu-devel] [PATCH 2/2] slirp: use socket_set_nonblock

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 slirp/misc.c     |   46 +---------------------------------------------
 slirp/tcp_subr.c |    4 ++--
 2 files changed, 3 insertions(+), 47 deletions(-)

diff --git a/slirp/misc.c b/slirp/misc.c
index 0308a62..0bee864 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -215,7 +215,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                 setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, (char *)&opt, sizeof(int));
                 opt = 1;
                 setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, (char *)&opt, sizeof(int));
-		fd_nonblock(so->s);
+		socket_set_nonblock(so->s);
 
 		/* Append the telnet options now */
                 if (so->so_m != NULL && do_pty == 1)  {
@@ -267,50 +267,6 @@ u_sleep(int usec)
 	select(0, &fdset, &fdset, &fdset, &t);
 }
 
-/*
- * Set fd blocking and non-blocking
- */
-
-void
-fd_nonblock(int fd)
-{
-#ifdef FIONBIO
-#ifdef _WIN32
-        unsigned long opt = 1;
-#else
-        int opt = 1;
-#endif
-
-	ioctlsocket(fd, FIONBIO, &opt);
-#else
-	int opt;
-
-	opt = fcntl(fd, F_GETFL, 0);
-	opt |= O_NONBLOCK;
-	fcntl(fd, F_SETFL, opt);
-#endif
-}
-
-void
-fd_block(int fd)
-{
-#ifdef FIONBIO
-#ifdef _WIN32
-        unsigned long opt = 0;
-#else
-	int opt = 0;
-#endif
-
-	ioctlsocket(fd, FIONBIO, &opt);
-#else
-	int opt;
-
-	opt = fcntl(fd, F_GETFL, 0);
-	opt &= ~O_NONBLOCK;
-	fcntl(fd, F_SETFL, opt);
-#endif
-}
-
 void slirp_connection_info(Slirp *slirp, Monitor *mon)
 {
     const char * const tcpstates[] = {
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 6f6585a..0a545c4 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -336,7 +336,7 @@ int tcp_fconnect(struct socket *so)
     int opt, s=so->s;
     struct sockaddr_in addr;
 
-    fd_nonblock(s);
+    socket_set_nonblock(s);
     opt = 1;
     setsockopt(s,SOL_SOCKET,SO_REUSEADDR,(char *)&opt,sizeof(opt ));
     opt = 1;
@@ -424,7 +424,7 @@ tcp_connect(struct socket *inso)
 		tcp_close(sototcpcb(so)); /* This will sofree() as well */
 		return;
 	}
-	fd_nonblock(s);
+	socket_set_nonblock(s);
 	opt = 1;
 	setsockopt(s,SOL_SOCKET,SO_REUSEADDR,(char *)&opt,sizeof(int));
 	opt = 1;
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 1/2] slirp: clean up conflicts with system headers

[Jan Kiszka]

On 2012-03-22 01:02, Paolo Bonzini wrote:
> Right now, slirp/slirp.h cannot include some system headers and,
> indirectly, qemu_socket.h.  Clean this up, and remove a duplicate
> prototype that was introduced because of that.
> 
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  slirp/slirp.h |    8 +-------
>  slirp/tcp.h   |   21 +++++++++++++++------
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 5033ee3..2098b20 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -88,10 +88,6 @@ void *malloc(size_t arg);
>  void free(void *ptr);
>  #endif
>  
> -#ifndef HAVE_INET_ATON
> -int inet_aton(const char *cp, struct in_addr *ia);
> -#endif
> -
>  #include <fcntl.h>
>  #ifndef NO_UNIX_SOCKETS
>  #include <sys/un.h>
> @@ -144,6 +140,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
>  #include "debug.h"
>  
>  #include "qemu-queue.h"
> +#include "qemu_socket.h"
>  
>  #include "libslirp.h"
>  #include "ip.h"
> @@ -167,9 +164,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
>  #include "bootp.h"
>  #include "tftp.h"
>  
> -/* osdep.c */
> -int qemu_socket(int domain, int type, int protocol);
> -
>  #define ETH_ALEN 6
>  #define ETH_HLEN 14
>  
> diff --git a/slirp/tcp.h b/slirp/tcp.h
> index b3817cb..8299603 100644
> --- a/slirp/tcp.h
> +++ b/slirp/tcp.h
> @@ -45,6 +45,7 @@ typedef	uint32_t tcp_seq;
>   * TCP header.
>   * Per RFC 793, September, 1981.
>   */
> +#define tcphdr slirp_tcphdr

Nice :). What about s/tcphdr/bsd_tcphdr/ or so for all slirp files? Even
better would be enabling slirp to use an existing declaration. But that
looks trickier in first sight.

>  struct tcphdr {
>  	uint16_t th_sport;              /* source port */
>  	uint16_t th_dport;              /* destination port */
> @@ -58,12 +59,6 @@ struct tcphdr {
>  		th_off:4;		/* data offset */
>  #endif
>  	uint8_t th_flags;
> -#define	TH_FIN	0x01
> -#define	TH_SYN	0x02
> -#define	TH_RST	0x04
> -#define	TH_PUSH	0x08
> -#define	TH_ACK	0x10
> -#define	TH_URG	0x20
>  	uint16_t th_win;                /* window */
>  	uint16_t th_sum;                /* checksum */
>  	uint16_t th_urp;                /* urgent pointer */
> @@ -71,6 +66,16 @@ struct tcphdr {
>  
>  #include "tcp_var.h"
>  
> +#ifndef TH_FIN
> +#define	TH_FIN	0x01
> +#define	TH_SYN	0x02
> +#define	TH_RST	0x04
> +#define	TH_PUSH	0x08
> +#define	TH_ACK	0x10
> +#define	TH_URG	0x20
> +#endif
> +
> +#ifndef TCPOPT_EOL
>  #define	TCPOPT_EOL		0
>  #define	TCPOPT_NOP		1
>  #define	TCPOPT_MAXSEG		2
> @@ -86,6 +91,7 @@ struct tcphdr {
>  
>  #define TCPOPT_TSTAMP_HDR	\
>      (TCPOPT_NOP<<24|TCPOPT_NOP<<16|TCPOPT_TIMESTAMP<<8|TCPOLEN_TIMESTAMP)
> +#endif

Is there no portable header that offers those defines for us?

>  
>  /*
>   * Default maximum segment size for TCP.
> @@ -95,10 +101,13 @@ struct tcphdr {
>   *
>   * We make this 1460 because we only care about Ethernet in the qemu context.
>   */
> +#undef TCP_MSS
>  #define	TCP_MSS	1460
>  
> +#undef TCP_MAXWIN
>  #define	TCP_MAXWIN	65535	/* largest value for (unscaled) window */
>  
> +#undef TCP_MAX_WINSHIFT
>  #define TCP_MAX_WINSHIFT	14	/* maximum window shift */
>  
>  /*

Same here.

The direction is appreciated a lot, but I'm measuring only a moderate
overall hack-level reduction. ;)

Patch 2 looks ok, btw.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/2] slirp: clean up conflicts with system headers

[Paolo Bonzini]

Il 22/03/2012 15:35, Jan Kiszka ha scritto:
>> @@ -167,9 +164,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
>>  #include "bootp.h"
>>  #include "tftp.h"
>>  
>> -/* osdep.c */
>> -int qemu_socket(int domain, int type, int protocol);
>> -
>>  #define ETH_ALEN 6
>>  #define ETH_HLEN 14
>>  
>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>> index b3817cb..8299603 100644
>> --- a/slirp/tcp.h
>> +++ b/slirp/tcp.h
>> @@ -45,6 +45,7 @@ typedef	uint32_t tcp_seq;
>>   * TCP header.
>>   * Per RFC 793, September, 1981.
>>   */
>> +#define tcphdr slirp_tcphdr
> 
> Nice :). What about s/tcphdr/bsd_tcphdr/ or so for all slirp files?

Well, we have a precedent here:

/* Avoid conflicting with the libc insque() and remque(), which
   have different prototypes. */
#define insque slirp_insque
#define remque slirp_remque

> Even better would be enabling slirp to use an existing declaration. But that
> looks trickier in first sight.

Yep, especially with no Autoconf.

>>  struct tcphdr {
>>  	uint16_t th_sport;              /* source port */
>>  	uint16_t th_dport;              /* destination port */
>> @@ -58,12 +59,6 @@ struct tcphdr {
>>  		th_off:4;		/* data offset */
>>  #endif
>>  	uint8_t th_flags;
>> -#define	TH_FIN	0x01
>> -#define	TH_SYN	0x02
>> -#define	TH_RST	0x04
>> -#define	TH_PUSH	0x08
>> -#define	TH_ACK	0x10
>> -#define	TH_URG	0x20
>>  	uint16_t th_win;                /* window */
>>  	uint16_t th_sum;                /* checksum */
>>  	uint16_t th_urp;                /* urgent pointer */
>> @@ -71,6 +66,16 @@ struct tcphdr {
>>  
>>  #include "tcp_var.h"
>>  
>> +#ifndef TH_FIN
>> +#define	TH_FIN	0x01
>> +#define	TH_SYN	0x02
>> +#define	TH_RST	0x04
>> +#define	TH_PUSH	0x08
>> +#define	TH_ACK	0x10
>> +#define	TH_URG	0x20
>> +#endif
>> +
>> +#ifndef TCPOPT_EOL
>>  #define	TCPOPT_EOL		0
>>  #define	TCPOPT_NOP		1
>>  #define	TCPOPT_MAXSEG		2
>> @@ -86,6 +91,7 @@ struct tcphdr {
>>  
>>  #define TCPOPT_TSTAMP_HDR	\
>>      (TCPOPT_NOP<<24|TCPOPT_NOP<<16|TCPOPT_TIMESTAMP<<8|TCPOLEN_TIMESTAMP)
>> +#endif
> 
> Is there no portable header that offers those defines for us?

That would be netinet/tcp.h, but the problem is that there are some
differences.  For example TCP_MSS is usually 512.

BTW the same could happen for ip.h, it's only that we never include it.
We need to include netinet/tcp.h outside slirp for TCP_NODELAY.

>>  /*
>>   * Default maximum segment size for TCP.
>> @@ -95,10 +101,13 @@ struct tcphdr {
>>   *
>>   * We make this 1460 because we only care about Ethernet in the qemu context.
>>   */
>> +#undef TCP_MSS
>>  #define	TCP_MSS	1460
>>  
>> +#undef TCP_MAXWIN
>>  #define	TCP_MAXWIN	65535	/* largest value for (unscaled) window */
>>  
>> +#undef TCP_MAX_WINSHIFT
>>  #define TCP_MAX_WINSHIFT	14	/* maximum window shift */
>>  
>>  /*
> 
> Same here.
> 
> The direction is appreciated a lot, but I'm measuring only a moderate
> overall hack-level reduction. ;)

Agreed, but one step at a time... this patch sticks to what it
promises, "clean up conflicts with system headers". :)

Paolo

Re: [Qemu-devel] [PATCH 1/2] slirp: clean up conflicts with system headers

[Jan Kiszka]

On 2012-03-22 20:24, Paolo Bonzini wrote:
> Il 22/03/2012 15:35, Jan Kiszka ha scritto:
>>> @@ -167,9 +164,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
>>>  #include "bootp.h"
>>>  #include "tftp.h"
>>>  
>>> -/* osdep.c */
>>> -int qemu_socket(int domain, int type, int protocol);
>>> -
>>>  #define ETH_ALEN 6
>>>  #define ETH_HLEN 14
>>>  
>>> diff --git a/slirp/tcp.h b/slirp/tcp.h
>>> index b3817cb..8299603 100644
>>> --- a/slirp/tcp.h
>>> +++ b/slirp/tcp.h
>>> @@ -45,6 +45,7 @@ typedef	uint32_t tcp_seq;
>>>   * TCP header.
>>>   * Per RFC 793, September, 1981.
>>>   */
>>> +#define tcphdr slirp_tcphdr
>>
>> Nice :). What about s/tcphdr/bsd_tcphdr/ or so for all slirp files?
> 
> Well, we have a precedent here:
> 
> /* Avoid conflicting with the libc insque() and remque(), which
>    have different prototypes. */
> #define insque slirp_insque
> #define remque slirp_remque

I know...

> 
>> Even better would be enabling slirp to use an existing declaration. But that
>> looks trickier in first sight.
> 
> Yep, especially with no Autoconf.
> 
>>>  struct tcphdr {
>>>  	uint16_t th_sport;              /* source port */
>>>  	uint16_t th_dport;              /* destination port */
>>> @@ -58,12 +59,6 @@ struct tcphdr {
>>>  		th_off:4;		/* data offset */
>>>  #endif
>>>  	uint8_t th_flags;
>>> -#define	TH_FIN	0x01
>>> -#define	TH_SYN	0x02
>>> -#define	TH_RST	0x04
>>> -#define	TH_PUSH	0x08
>>> -#define	TH_ACK	0x10
>>> -#define	TH_URG	0x20
>>>  	uint16_t th_win;                /* window */
>>>  	uint16_t th_sum;                /* checksum */
>>>  	uint16_t th_urp;                /* urgent pointer */
>>> @@ -71,6 +66,16 @@ struct tcphdr {
>>>  
>>>  #include "tcp_var.h"
>>>  
>>> +#ifndef TH_FIN
>>> +#define	TH_FIN	0x01
>>> +#define	TH_SYN	0x02
>>> +#define	TH_RST	0x04
>>> +#define	TH_PUSH	0x08
>>> +#define	TH_ACK	0x10
>>> +#define	TH_URG	0x20
>>> +#endif
>>> +
>>> +#ifndef TCPOPT_EOL
>>>  #define	TCPOPT_EOL		0
>>>  #define	TCPOPT_NOP		1
>>>  #define	TCPOPT_MAXSEG		2
>>> @@ -86,6 +91,7 @@ struct tcphdr {
>>>  
>>>  #define TCPOPT_TSTAMP_HDR	\
>>>      (TCPOPT_NOP<<24|TCPOPT_NOP<<16|TCPOPT_TIMESTAMP<<8|TCPOLEN_TIMESTAMP)
>>> +#endif
>>
>> Is there no portable header that offers those defines for us?
> 
> That would be netinet/tcp.h, but the problem is that there are some
> differences.  For example TCP_MSS is usually 512.
> 
> BTW the same could happen for ip.h, it's only that we never include it.
> We need to include netinet/tcp.h outside slirp for TCP_NODELAY.
> 
>>>  /*
>>>   * Default maximum segment size for TCP.
>>> @@ -95,10 +101,13 @@ struct tcphdr {
>>>   *
>>>   * We make this 1460 because we only care about Ethernet in the qemu context.
>>>   */
>>> +#undef TCP_MSS
>>>  #define	TCP_MSS	1460
>>>  
>>> +#undef TCP_MAXWIN
>>>  #define	TCP_MAXWIN	65535	/* largest value for (unscaled) window */
>>>  
>>> +#undef TCP_MAX_WINSHIFT
>>>  #define TCP_MAX_WINSHIFT	14	/* maximum window shift */
>>>  
>>>  /*
>>
>> Same here.
>>
>> The direction is appreciated a lot, but I'm measuring only a moderate
>> overall hack-level reduction. ;)
> 
> Agreed, but one step at a time... this patch sticks to what it
> promises, "clean up conflicts with system headers". :)

Yeah, given that there is no sufficiently simple alternative, I guess I
have to accept this as is. :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/2] slirp: remove fd_block/fd_nonblock

[Jan Kiszka]

On 2012-03-22 01:02, Paolo Bonzini wrote:
> Of these two functions, one is unused and the other is a dup of
> socket_set_nonblock.  Unfortunately, there are some conflicts
> with system headers that need to be sorted out first.
> 
> Paolo Bonzini (2):
>   slirp: clean up conflicts with system headers
>   slirp: use socket_set_nonblock
> 
>  slirp/misc.c     |   46 +---------------------------------------------
>  slirp/slirp.h    |    8 +-------
>  slirp/tcp.h      |   21 +++++++++++++++------
>  slirp/tcp_subr.c |    4 ++--
>  4 files changed, 19 insertions(+), 60 deletions(-)
> 

Thanks, applied to the slirp queue.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux