[Qemu-devel] [PULL 2/9] slirp/smb: Replace constant strings by glib string

[Samuel Thibault]

From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>

gcc 7 (on fedora 26) objects to many of the snprintf's
in the smb path and command creation because it can't
figure out that the smb_dir (i.e. the /tmp dir for the configuration)
is known to be short.

Replace all these fixed length buffers by g_str* functions that dynamically
allocate and use g_dir_make_tmp to make the directory.
(It's fairly new glib but we have a compat function for it).

Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 net/slirp.c | 30 +-
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index 11b2dd249a..c705a60b62 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -80,7 +80,7 @@ typedef struct SlirpState {
 Slirp *slirp;
 Notifier exit_notifier;
 #ifndef _WIN32
-char smb_dir[128];
+gchar *smb_dir;
 #endif
 } SlirpState;
 
@@ -558,11 +558,10 @@ int net_slirp_redir(const char *redir_str)
 /* automatic user mode samba server configuration */
 static void slirp_smb_cleanup(SlirpState *s)
 {
-char cmd[128];
 int ret;
 
-if (s->smb_dir[0] != '\0') {
-snprintf(cmd, sizeof(cmd), "rm -rf %s", s->smb_dir);
+if (s->smb_dir) {
+gchar *cmd = g_strdup_printf("rm -rf %s", s->smb_dir);
 ret = system(cmd);
 if (ret == -1 || !WIFEXITED(ret)) {
 error_report("'%s' failed.", cmd);
@@ -570,15 +569,17 @@ static void slirp_smb_cleanup(SlirpState *s)
 error_report("'%s' failed. Error code: %d",
  cmd, WEXITSTATUS(ret));
 }
-s->smb_dir[0] = '\0';
+g_free(cmd);
+g_free(s->smb_dir);
+s->smb_dir = NULL;
 }
 }
 
 static int slirp_smb(SlirpState* s, const char *exported_dir,
  struct in_addr vserver_addr)
 {
-char smb_conf[128];
-char smb_cmdline[128];
+char *smb_conf;
+char *smb_cmdline;
 struct passwd *passwd;
 FILE *f;
 
@@ -600,19 +601,19 @@ static int slirp_smb(SlirpState* s, const char 
*exported_dir,
 return -1;
 }
 
-snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.XX");
-if (!mkdtemp(s->smb_dir)) {
-error_report("could not create samba server dir '%s'", s->smb_dir);
-s->smb_dir[0] = 0;
+s->smb_dir = g_dir_make_tmp("qemu-smb.XX", NULL);
+if (!s->smb_dir) {
+error_report("could not create samba server dir");
 return -1;
 }
-snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir, "smb.conf");
+smb_conf = g_strdup_printf("%s/%s", s->smb_dir, "smb.conf");
 
 f = fopen(smb_conf, "w");
 if (!f) {
 slirp_smb_cleanup(s);
 error_report("could not create samba server configuration file '%s'",
  smb_conf);
+g_free(smb_conf);
 return -1;
 }
 fprintf(f,
@@ -651,15 +652,18 @@ static int slirp_smb(SlirpState* s, const char 
*exported_dir,
 );
 fclose(f);
 
-snprintf(smb_cmdline, sizeof(smb_cmdline), "%s -l %s -s %s",
+smb_cmdline = g_strdup_printf("%s -l %s -s %s",
  CONFIG_SMBD_COMMAND, s->smb_dir, smb_conf);
+g_free(smb_conf);
 
 if (slirp_add_exec(s->slirp, 0, smb_cmdline, _addr, 139) < 0 ||
 slirp_add_exec(s->slirp, 0, smb_cmdline, _addr, 445) < 0) {
 slirp_smb_cleanup(s);
+g_free(smb_cmdline);
 error_report("conflicting/invalid smbserver address");
 return -1;
 }
+g_free(smb_cmdline);
 return 0;
 }
 
-- 
2.11.0

[Qemu-devel] [PULL 3/9] slirp: tftp, copy sockaddr_size

[Samuel Thibault]

From: Marc-André Lureau <marcandre.lur...@redhat.com>

ASAN detects an "unknown-crash" when running pxe-test:

/ppc64/pxe/spapr-vlan: 
=
==7143==ERROR: AddressSanitizer: unknown-crash on address 0x7f6dcd298d30 at pc 
0x55e22218830d bp 0x7f6dcd2989e0 sp 0x7f6dcd2989d0
READ of size 128 at 0x7f6dcd298d30 thread T2
#0 0x55e22218830c in tftp_session_allocate 
/home/elmarco/src/qq/slirp/tftp.c:73
#1 0x55e22218a1f8 in tftp_handle_rrq /home/elmarco/src/qq/slirp/tftp.c:289
#2 0x55e22218b54c in tftp_input /home/elmarco/src/qq/slirp/tftp.c:446
#3 0x55e2221833fe in udp6_input /home/elmarco/src/qq/slirp/udp6.c:82
#4 0x55e222137b17 in ip6_input /home/elmarco/src/qq/slirp/ip6_input.c:67

Address 0x7f6dcd298d30 is located in stack of thread T2 at offset 96 in frame
#0 0x55e222182420 in udp6_input /home/elmarco/src/qq/slirp/udp6.c:13

  This frame has 3 object(s):
[32, 48) ''
[96, 124) 'lhost' <== Memory access at offset 96 partially overflows this 
variable
[160, 200) 'save_ip' <== Memory access at offset 96 partially underflows 
this variable

The sockaddr_storage pointer is the sockaddr_in6 lhost on the
stack. Copy only the source addr size.

Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
Reviewed-by: Thomas Huth <th...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/tftp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index 50e714807d..a9bc4bb1b6 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -70,7 +70,7 @@ static int tftp_session_allocate(Slirp *slirp, struct 
sockaddr_storage *srcsas,
 
  found:
   memset(spt, 0, sizeof(*spt));
-  spt->client_addr = *srcsas;
+  memcpy(>client_addr, srcsas, sockaddr_size(srcsas));
   spt->fd = -1;
   spt->block_size = 512;
   spt->client_port = tp->udp.uh_sport;
-- 
2.11.0

[Qemu-devel] [PULL 1/9] slirp: allow host port 0 for hostfwd

[Samuel Thibault]

From: Vincent Bernat <vinc...@bernat.im>

The OS will allocate automatically a free port. This is useful if you
want to be sure to not get any port conflict. You still have to figure
out which port you got, for example with "lsof" (this could be exposed
in the monitor if needed).

Example of use:

 $ qemu-system-x86_64 -net user,hostfwd=127.0.0.1:0-:22 ...

Then, get your port with:

 $ lsof -np 1474 | grep LISTEN
 qemu-syst 31777 bernat 12u IPv4 [...] TCP 127.0.0.1:35145 (LISTEN)

Signed-off-by: Vincent Bernat <vinc...@bernat.im>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 net/slirp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/slirp.c b/net/slirp.c
index f97ec23345..11b2dd249a 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -487,7 +487,7 @@ static int slirp_hostfwd(SlirpState *s, const char 
*redir_str,
 goto fail_syntax;
 }
 host_port = strtol(buf, , 0);
-if (*end != '\0' || host_port < 1 || host_port > 65535) {
+if (*end != '\0' || host_port < 0 || host_port > 65535) {
 goto fail_syntax;
 }
 
-- 
2.11.0

[Qemu-devel] [PULL 0/9] slirp updates

[Samuel Thibault]

The following changes since commit 81b2d5ceb0cfb4cdc2163492e3169ed714b0cda9:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20170426' into 
staging (2017-04-26 20:50:49 +0100)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to eb5d4f5329df83ea15244b47f7fbca21adaae41b:

  slirp: VMStatify remaining except for loop (2017-04-29 18:44:16 +0200)


slirp updates


Dr. David Alan Gilbert (6):
  slirp/smb: Replace constant strings by glib string
  slirp: VMState conversion; tcpcb
  slirp: VMStatify sbuf
  slirp: Common lhost/fhost union
  slirp: VMStatify socket level
  slirp: VMStatify remaining except for loop

Marc-André Lureau (1):
  slirp: tftp, copy sockaddr_size

Samuel Thibault (1):
  slirp: fix pinging the virtual ipv4 DNS server

Vincent Bernat (1):
  slirp: allow host port 0 for hostfwd

 net/slirp.c |  32 ++--
 slirp/ip_icmp.c |   5 +-
 slirp/sbuf.h|   4 +-
 slirp/slirp.c   | 494 +++-
 slirp/socket.h  |  24 ++-
 slirp/tcp_var.h |   6 +-
 slirp/tftp.c|   2 +-
 7 files changed, 310 insertions(+), 257 deletions(-)

[Qemu-devel] [PATCH] slirp: fix pinging the virtual ipv4 DNS server

[Samuel Thibault]

so that people do not think it is not working at least basically.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 5ffc7a683d..0b667a429a 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -152,8 +152,9 @@ icmp_input(struct mbuf *m, int hlen)
   switch (icp->icmp_type) {
   case ICMP_ECHO:
 ip->ip_len += hlen; /* since ip_input subtracts this */
-if (ip->ip_dst.s_addr == slirp->vhost_addr.s_addr) {
-  icmp_reflect(m);
+if (ip->ip_dst.s_addr == slirp->vhost_addr.s_addr ||
+ip->ip_dst.s_addr == slirp->vnameserver_addr.s_addr) {
+icmp_reflect(m);
 } else if (slirp->restricted) {
 goto freeit;
 } else {

Re: [Qemu-devel] dns server not working in QEMU using usermode networking (SLIRP)

[Samuel Thibault]

Hello,

FONNEMANN Mark, on mer. 19 avril 2017 18:11:59 +, wrote:
> I have tested 2.9-rc4 and the problem still exists there as well.

FONNEMANN Mark, on lun. 24 avril 2017 23:43:02 +, wrote:
> I just confirmed that the problem exists in 2.9 release using 
> qemu-system-i386.exe as well.

FONNEMANN Mark, on mer. 26 avril 2017 18:51:00 +, wrote:
> I just verified the problem exists on Linux host as well. I cannot ping the 
> DNS or SMB server and the DNS server does not respond to nslookup requests.

One first thing to note: the DNS or SMB servers can not be pinged, since
that was never implemented (I'll post a patch for that), so there is no
surprise on that end.

Also, your issue happens even with explictly requesting ipv4, so it's
probably not a problem with the v6 updates by themselves.

When running with a Linux host (since that's what I'm most familiar
with), what do you have in /etc/resolv.conf?

Samuel

Re: [Qemu-devel] [PATCH 1/1] slirp: don't zero ti_i since we acccess it later.

[Samuel Thibault]

Hello,

Thomas Huth, on lun. 24 avril 2017 11:15:56 +0200, wrote:
> On 20.04.2017 22:43, Tao Wu wrote:
> > The current code looks buggy, we zero ti_i while we access
> > ti_dst/ti_src later.

Indeed.

> > Signed-off-by: Tao Wu 

> > *mtod(m, struct tcpiphdr *) = *ti;
> > ti = mtod(m, struct tcpiphdr *);
> > -   memset(>ti, 0, sizeof(ti->ti));

But then we don't make sure that ih_x1 is 0, which is needed for the
checksum to be correct, but possibly not set by the caller.

So please replace the memset call with setting the proper ih_x1 field to
0 (which thus needs the introductino of a switch over af like below in
the code).

Samuel

Re: [Qemu-devel] dns server not working in QEMU using usermode networking (SLIRP)

[Samuel Thibault]

Stefan Weil, on ven. 21 avril 2017 21:58:18 +0200, wrote:
> Am 17.04.2017 um 00:10 schrieb FONNEMANN Mark:
> > I hadn't seen the original report on the list, sorry. There is too much 
> > traffic on qemu-devel for me to manage to catch these :/
> > 
> > This problem was fixed by
> > e42f869b ("slirp: Make RA build more flexible") and a2f80fdf ("slirp: Send 
> > RDNSS in RA only if host has an IPv6 DNS server") which will be included in 
> > qemu 2.9.
> > 
> > Samuel
> 
> See report on 
> http://stackoverflow.com/questions/43308310/dns-server-not-working-in-qemu-usermode-networking.
> 
> Mark, did you also get that problem with QEMU running on a Linux host,
> or is it specific for QEMU running on Windows?

Ah, I hadn't noticed the report was about windows. The abovementioned
fix should still be fine for windows: there, get_dns6_addr just always
returns -1, and thus the RDNSS option is never added.

So I don't know what to do on my side, more investigation is needed
there.

Samuel

Re: [Qemu-devel] dns server not working in QEMU using usermode networking (SLIRP)

[Samuel Thibault]

Hello,

I hadn't seen the original report on the list, sorry. There is too much
traffic on qemu-devel for me to manage to catch these :/

This problem was fixed by 
e42f869b ("slirp: Make RA build more flexible") and
a2f80fdf ("slirp: Send RDNSS in RA only if host has an IPv6 DNS server")
which will be included in qemu 2.9.

Samuel

Re: [Qemu-devel] [PATCH v2 5/5] slirp: add a fake NC-SI backend

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, on jeu. 13 avril 2017 08:45:23 -0300, wrote:
> > The NCSI header file  comes from mainline Linux

Please mention within the file which file it comes from exactly.

> > +case NCSI_PKT_CMD_SMA:
> > +rnh->common.length = htons(4);
> > +break;
> > +case NCSI_PKT_CMD_GVI:
> > +rnh->common.length = htons(36);
> > +break;
> > +case NCSI_PKT_CMD_GC: {
> > +rnh->common.length = htons(32);
...
> > +break;
> > +}
> > +
> > +case NCSI_PKT_CMD_GLS: {
> > +rnh->common.length = htons(16);
> > +break;
> > +}
> > +
> > +slirp_output(slirp->opaque, ncsi_reply, sizeof(ncsi_reply));

Are we really supposed to send sizeof(ncsi_reply), and not accordingly
to the size announced withing the packet?

Appart from that,

Acked-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

Samuel

Re: [Qemu-devel] [PATCH for 2.10] slirp/smb: Replace constant strings by glib string

[Samuel Thibault]

Hello,

Applied to my tree, thanks!

Samuel

Re: [Qemu-devel] [PATCH v2 1/1] slirp: add SOCKS5 support

[Samuel Thibault]

Hello,

Thanks for the patch!

Laurent Vivier, on mar. 28 mars 2017 21:07:56 +0200, wrote:
> @@ -617,6 +621,10 @@ void slirp_pollfds_poll(GArray *pollfds, int 
> select_error)
>   * Check sockets for reading
>   */
>  else if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) {
> +if (so->so_state & SS_ISFCONNECTING) {
> +socks5_recv(so->s, >so_proxy_state);
> +continue;
> +}

It looks odd to be calling socks5_recv in all cases.  Shouldn't this
check for so_proxy_state != 0?

> @@ -645,11 +653,19 @@ void slirp_pollfds_poll(GArray *pollfds, int 
> select_error)
>  /*
>   * Check for non-blocking, still-connecting sockets
>   */
> -if (so->so_state & SS_ISFCONNECTING) {
> -/* Connected */
> -so->so_state &= ~SS_ISFCONNECTING;
>  
> -ret = send(so->s, (const void *) , 0, 0);
> +if (so->so_state & SS_ISFCONNECTING) {
> +ret = socks5_send(so->s, slirp->proxy_user,

Ditto. Socks5 and non-socks5 code should be properly separated I guess.

> @@ -1069,6 +1085,48 @@ int slirp_add_exec(Slirp *slirp, int do_pty, const 
> void *args,
>  htons(guest_port));
>  }
>  
> +int slirp_add_proxy(Slirp *slirp, const char *server, int port,
> +const char *user, const char *password)
> +{
> +int fd;
> +socks5_state_t state;
> +struct sockaddr_storage addr;
> +
> +/* check the connection */

Please be a bit more verbose :) For instance:

> + /* just check that the connection to the socks5 server works with
> the given credentials, and close without doing anything with it. */



> diff --git a/slirp/socks5.c b/slirp/socks5.c
> new file mode 100644
> index 000..1c5e071
> --- /dev/null
> +++ b/slirp/socks5.c
> @@ -0,0 +1,284 @@
> +/* some parts from nmap/ncat GPLv2 */


One can't just steal code like this.  You *have* to copy over the
copyright notice, since there's notably the author information.

> +static int socks5_proxy_connect(int fd, const char *server, int port)
> +{
> +struct sockaddr_in saddr;
> +struct hostent *he;
> +
> +he = gethostbyname(server);
> +if (he == NULL) {
> +errno = EINVAL;
> +return -1;
> +}
> +saddr.sin_family = AF_INET;
> +saddr.sin_addr = *(struct in_addr *)he->h_addr;
> +saddr.sin_port = htons(port);

Please add a TODO: v6 version

> +static int socks5_recv_authenticate(int fd)
> +{
> +char socksbuf[2];
> +if (recv(fd, socksbuf, 2, 0) != 2) {
> +return -1;
> +}
> +if (socksbuf[0] != 1 || socksbuf[1] != 0) {

These look like magic numbers :) Please document what they represent.

> +int len;
> +
> +memset(, 0, sizeof(socks5msg));
> +
> +socks5msg.ver = SOCKS5_VERSION;
> +socks5msg.cmd = SOCKS_CONNECT;
> +socks5msg.rsv = 0;

Please rather set len to 4 here, and increment later on

> +switch (addr->ss_family) {
> +case AF_INET: {
> +struct sockaddr_in *a = (struct sockaddr_in *)addr;
> +
> +socks5msg.atyp = SOCKS5_ATYP_IPv4;
> +memcpy(socks5msg.dst, >sin_addr, sizeof(a->sin_addr));
> +len = sizeof(a->sin_addr);

here

> +memcpy(socks5msg.dst + len, >sin_port, sizeof(a->sin_port));
> +len += sizeof(a->sin_port);
> +}
> +break;
> +case AF_INET6: {
> +struct sockaddr_in6 *a = (struct sockaddr_in6 *)addr;
> +
> +socks5msg.atyp = SOCKS5_ATYP_IPv6;
> +memcpy(socks5msg.dst, >sin6_addr, sizeof(a->sin6_addr));
> +len = sizeof(a->sin6_addr);

and there.

> +memcpy(socks5msg.dst + len, >sin6_port, sizeof(a->sin6_port));
> +len += sizeof(a->sin6_port);
> +}
> +break;
> +default:
> +errno = EINVAL;
> +return -1;
> +}
> +len += 4;

and not have this here, where people wonder what that is :)

> +static int socks5_recv_connect(int fd)
> +{
> +struct socks5_request socks5msg;
> +
> +if (recv(fd, , 4, 0) != 4) {

Thinking about it (there are more like that above and below). You can
theoretically have a short read here... So please at least leave a note
that we actually should manage buffering of recv()s.

> +++ b/slirp/socks5.h
> +struct socks4_data {
> +char version;
> +char type;
> +unsigned short port;
> +uint32_t address;
> +char data[SOCKS_BUFF_SIZE]; /* to hold FQDN and username */
> +} __attribute__((packed));
> +
> +/* SOCKS4 protocol responses */
> +#define SOCKS4_VERSION  4
> +#define SOCKS_CONNECT   1
> +#define SOCKS_BIND  2
> +#define SOCKS4_CONN_ACC 90
> +#define SOCKS4_CONN_REF 91
> +#define SOCKS4_CONN_IDENT   92
> +#define SOCKS4_CONN_IDENTDIFF   93

Re: [Qemu-devel] [PATCH 14/43] usb: made printf always compile in debug output

[Samuel Thibault]

Danil Antonov, on sam. 01 avril 2017 16:48:58 +0300, wrote:
> From 02b1e378eea154c0169a3d16b64ae27c64919c2c Mon Sep 17 00:00:00 2001
> From: Danil Antonov <[1]g.danil.a...@gmail.com>
> Date: Wed, 29 Mar 2017 12:28:51 +0300
> Subject: [PATCH 14/43] usb: made printf always compile in debug output
> 
> Wrapped printf calls inside debug macros (DPRINTF) in `if` statement.
> This will ensure that printf function will always compile even if debug
> output is turned off and, in turn, will prevent bitrot of the format
> strings.
> 
> Signed-off-by: Danil Antonov <[2]g.danil.a...@gmail.com>

Acked-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

> ---
>  hw/usb/dev-serial.c  | 17 +
>  hw/usb/dev-storage.c | 17 +
>  hw/usb/hcd-ehci.h    | 10 +-
>  hw/usb/hcd-xhci.c    | 19 ---
>  4 files changed, 35 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 6d51373..739e79b 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -17,14 +17,15 @@
>  #include "hw/usb/desc.h"
>  #include "sysemu/char.h"
>  
> -//#define DEBUG_Serial
> -
> -#ifdef DEBUG_Serial
> -#define DPRINTF(fmt, ...) \
> -do { printf("usb-serial: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do {} while(0)
> -#endif
> +#ifndef DEBUG_Serial
> +#define DEBUG_Serial 0
> +#endif
> +
> +#define DPRINTF(fmt, ...) do {  \
> +    if (DEBUG_Serial) { \
> +    fprintf(stderr, "usb-serial: " fmt,## __VA_ARGS__); \
> +    }   \
> +} while (0);
>  
>  #define RECV_BUF 384
>  
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 8a61ec9..b85c006 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -24,14 +24,15 @@
>  #include "qapi/visitor.h"
>  #include "qemu/cutils.h"
>  
> -//#define DEBUG_MSD
> -
> -#ifdef DEBUG_MSD
> -#define DPRINTF(fmt, ...) \
> -do { printf("usb-msd: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) do {} while(0)
> -#endif
> +#ifndef DEBUG_MSD
> +#define DEBUG_MSD 0
> +#endif
> +
> +#define DPRINTF(fmt, ...) do { \
> +    if (DEBUG_MSD) {   \
> +    fprintf(stderr, "usb-msd: " fmt , ## __VA_ARGS__); \
> +    }  \
> +} while (0);
>  
>  /* USB requests.  */
>  #define MassStorageReset  0xff
> diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
> index 938d8aa..ca6094e 100644
> --- a/hw/usb/hcd-ehci.h
> +++ b/hw/usb/hcd-ehci.h
> @@ -30,11 +30,11 @@
>  #define EHCI_DEBUG   0
>  #endif
>  
> -#if EHCI_DEBUG
> -#define DPRINTF printf
> -#else
> -#define DPRINTF(...)
> -#endif
> +#define DPRINTF(fmt, ...) do {    \
> +    if (EHCI_DEBUG) { \
> +    fprintf(stderr, fmt, ## __VA_ARGS__); \
> +    } \
> +} while (0);
>  
>  #define MMIO_SIZE    0x1000
>  #define CAPA_SIZE    0x10
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index f0af852..9f5e5b2 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -32,11 +32,16 @@
>  //#define DEBUG_XHCI
>  //#define DEBUG_DATA
>  
> -#ifdef DEBUG_XHCI
> -#define DPRINTF(...) fprintf(stderr, __VA_ARGS__)
> -#else
> -#define DPRINTF(...) do {} while (0)
> -#endif
> +#ifndef DEBUG_XHCI
> +#define DEBUG_XHCI 0
> +#endif
> +
> +#define DPRINTF(fmt, ...) do { \
> +    if (DEBUG_XHCI) {  \
> +    fprintf(stderr, fmt , ## __VA_ARGS__); \
> +    }  \
> +} while (0);
> +
>  #define FIXME(_msg) do { fprintf(stderr, "FIXME %s:%d %s\n", \
>   __func__, __LINE__, _msg); abort(); } while
> (0)
>  
> @@ -1806,7 +1811,7 @@ static int xhci_setup_packet(XHCITransfer *xfer)
>  ep = xhci_epid_to_usbep(xfer->epctx);
>  if (!ep) {
>  DPRINTF("xhci: slot %d has no device\n",
> -    xfer->slotid);
> +    xfer->epctx->slotid);
>  return -1;
>  }
>  }
> @@ -1980,7 +1985,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer
> *xfer, XHCIEPContext *epctx
>  {
>  uint64_t mfindex;
>  
> -    DPRINTF("xhci_submit(slotid=%d,epid=%d)\n", xfer->slotid, xfer->epid);
> +    DPRINTF("xhci_submit(slotid=%d,epid=%d)\n", xfer->epctx->slotid, xfer->
> epctx->epid);
>  
>  xfer->in_xfer = epctx->type>>2;
>  
> --
> 2.8.0.rc3
> 
> 
> References:
> 
> [1] mailto:g.danil.a...@gmail.com
> [2] mailto:g.danil.a...@gmail.com

-- 
Samuel
c> [ ] morning [ ] afternoon [ ] evening [ ] night , everyone (choose as 
applicable)

[Qemu-devel] [PULL 2/3] slirp: Make RA build more flexible

[Samuel Thibault]

Do not hardcode the RA size at all, use a pl_size variable which
accounts the accumulated size, and fill rip->ip_pl at the end.

This will allow to make some blocks optional.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
---
 slirp/ip6_icmp.c | 24 +---
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index 298a48dd25..d0f5cc1456 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -143,17 +143,10 @@ void ndp_send_ra(Slirp *slirp)
 /* Build IPv6 packet */
 struct mbuf *t = m_get(slirp);
 struct ip6 *rip = mtod(t, struct ip6 *);
+size_t pl_size = 0;
 rip->ip_src = (struct in6_addr)LINKLOCAL_ADDR;
 rip->ip_dst = (struct in6_addr)ALLNODES_MULTICAST;
 rip->ip_nh = IPPROTO_ICMPV6;
-rip->ip_pl = htons(ICMP6_NDP_RA_MINLEN
-+ NDPOPT_LINKLAYER_LEN
-+ NDPOPT_PREFIXINFO_LEN
-#ifndef _WIN32
-+ NDPOPT_RDNSS_LEN
-#endif
-);
-t->m_len = sizeof(struct ip6) + ntohs(rip->ip_pl);
 
 /* Build ICMPv6 packet */
 t->m_data += sizeof(struct ip6);
@@ -171,6 +164,7 @@ void ndp_send_ra(Slirp *slirp)
 ricmp->icmp6_nra.reach_time = htonl(NDP_AdvReachableTime);
 ricmp->icmp6_nra.retrans_time = htonl(NDP_AdvRetransTime);
 t->m_data += ICMP6_NDP_RA_MINLEN;
+pl_size += ICMP6_NDP_RA_MINLEN;
 
 /* Source link-layer address (NDP option) */
 struct ndpopt *opt = mtod(t, struct ndpopt *);
@@ -178,6 +172,7 @@ void ndp_send_ra(Slirp *slirp)
 opt->ndpopt_len = NDPOPT_LINKLAYER_LEN / 8;
 in6_compute_ethaddr(rip->ip_src, opt->ndpopt_linklayer);
 t->m_data += NDPOPT_LINKLAYER_LEN;
+pl_size += NDPOPT_LINKLAYER_LEN;
 
 /* Prefix information (NDP option) */
 struct ndpopt *opt2 = mtod(t, struct ndpopt *);
@@ -192,6 +187,7 @@ void ndp_send_ra(Slirp *slirp)
 opt2->ndpopt_prefixinfo.reserved2 = 0;
 opt2->ndpopt_prefixinfo.prefix = slirp->vprefix_addr6;
 t->m_data += NDPOPT_PREFIXINFO_LEN;
+pl_size += NDPOPT_PREFIXINFO_LEN;
 
 #ifndef _WIN32
 /* Prefix information (NDP option) */
@@ -203,16 +199,14 @@ void ndp_send_ra(Slirp *slirp)
 opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
 opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
 t->m_data += NDPOPT_RDNSS_LEN;
+pl_size += NDPOPT_RDNSS_LEN;
 #endif
 
+rip->ip_pl = htons(pl_size);
+t->m_data -= sizeof(struct ip6) + pl_size;
+t->m_len = sizeof(struct ip6) + pl_size;
+
 /* ICMPv6 Checksum */
-#ifndef _WIN32
-t->m_data -= NDPOPT_RDNSS_LEN;
-#endif
-t->m_data -= NDPOPT_PREFIXINFO_LEN;
-t->m_data -= NDPOPT_LINKLAYER_LEN;
-t->m_data -= ICMP6_NDP_RA_MINLEN;
-t->m_data -= sizeof(struct ip6);
 ricmp->icmp6_cksum = ip6_cksum(t);
 
 ip6_output(NULL, t, 0);
-- 
2.11.0

[Qemu-devel] [PULL 1/3] slirp: fix compilation errors with DEBUG set

[Samuel Thibault]

From: Laurent Vivier <laur...@vivier.eu>

slirp/slirp.c: In function 'get_dns_addr_resolv_conf':
slirp/slirp.c:202:29: error: initialization discards 'const' qualifier from 
pointer target type [-Werror=discarded-qualifiers]
 char *res = inet_ntop(af, tmp_addr, s, sizeof(s));
 ^
slirp/slirp.c:204:25: error: assignment discards 'const' qualifier from pointer 
target type [-Werror=discarded-qualifiers]
 res = "(string conversion error)";

Signed-off-by: Laurent Vivier <laur...@vivier.eu>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/slirp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 60539de7a3..5a94b06f5e 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -198,7 +198,7 @@ static int get_dns_addr_resolv_conf(int af, void 
*pdns_addr, void *cached_addr,
 #ifdef DEBUG
 else {
 char s[INET6_ADDRSTRLEN];
-char *res = inet_ntop(af, tmp_addr, s, sizeof(s));
+const char *res = inet_ntop(af, tmp_addr, s, sizeof(s));
 if (!res) {
 res = "(string conversion error)";
 }
-- 
2.11.0

[Qemu-devel] [PULL 3/3] slirp: Send RDNSS in RA only if host has an IPv6 DNS server

[Samuel Thibault]

Previously we would always send an RDNSS option in the RA, making the guest
try to resolve DNS through IPv6, even if the host does not actually have
and IPv6 DNS server available.

This makes the RDNSS option enabled only when an IPv6 DNS server is
available.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
---
 slirp/ip6_icmp.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index d0f5cc1456..777eb574be 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -144,6 +144,9 @@ void ndp_send_ra(Slirp *slirp)
 struct mbuf *t = m_get(slirp);
 struct ip6 *rip = mtod(t, struct ip6 *);
 size_t pl_size = 0;
+struct in6_addr addr;
+uint32_t scope_id;
+
 rip->ip_src = (struct in6_addr)LINKLOCAL_ADDR;
 rip->ip_dst = (struct in6_addr)ALLNODES_MULTICAST;
 rip->ip_nh = IPPROTO_ICMPV6;
@@ -189,18 +192,18 @@ void ndp_send_ra(Slirp *slirp)
 t->m_data += NDPOPT_PREFIXINFO_LEN;
 pl_size += NDPOPT_PREFIXINFO_LEN;
 
-#ifndef _WIN32
 /* Prefix information (NDP option) */
-/* disabled for windows for now, until get_dns6_addr is implemented */
-struct ndpopt *opt3 = mtod(t, struct ndpopt *);
-opt3->ndpopt_type = NDPOPT_RDNSS;
-opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
-opt3->ndpopt_rdnss.reserved = 0;
-opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
-opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
-t->m_data += NDPOPT_RDNSS_LEN;
-pl_size += NDPOPT_RDNSS_LEN;
-#endif
+if (get_dns6_addr(, _id) >= 0) {
+/* Host system does have an IPv6 DNS server, announce our proxy.  */
+struct ndpopt *opt3 = mtod(t, struct ndpopt *);
+opt3->ndpopt_type = NDPOPT_RDNSS;
+opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
+opt3->ndpopt_rdnss.reserved = 0;
+opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
+opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
+t->m_data += NDPOPT_RDNSS_LEN;
+pl_size += NDPOPT_RDNSS_LEN;
+}
 
 rip->ip_pl = htons(pl_size);
 t->m_data -= sizeof(struct ip6) + pl_size;
-- 
2.11.0

[Qemu-devel] [PULL 0/3] slirp updates

[Samuel Thibault]

The following changes since commit df9046363220e57d45818312759b954c033c58ab:

  Update version for v2.9.0-rc2 release (2017-03-28 19:11:16 +0100)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to a2f80fdfc683019901cdf4c0863a5920c0ca7245:

  slirp: Send RDNSS in RA only if host has an IPv6 DNS server (2017-03-29 
00:51:25 +0200)


slirp updates


Laurent Vivier (1):
  slirp: fix compilation errors with DEBUG set

Samuel Thibault (2):
  slirp: Make RA build more flexible
  slirp: Send RDNSS in RA only if host has an IPv6 DNS server

 slirp/ip6_icmp.c | 47 ++-
 slirp/slirp.c|  2 +-
 2 files changed, 23 insertions(+), 26 deletions(-)

Re: [Qemu-devel] [PATCH 2/2] slirp: Send RDNSS in RA only if host has an IPv6 DNS server

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, on lun. 27 mars 2017 11:56:00 -0300, wrote:
> Why don't declare at function begining and remove this { } ?

Oh, right, now I can.  While working on the code I still had ifdef
WIN32, so it'd lead to an unused variable warning.  But now that the
ifdef is gone, we can just put the variable at the beginning of the
function indeed.

Thanks,
Samuel

[Qemu-devel] [PATCH 1/2] slirp: Make RA build more flexible

[Samuel Thibault]

Do not hardcode the RA size at all, use a pl_size variable which
accounts the accumulated size, and fill rip->ip_pl at the end.

This will allow to make some blocks optional.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/ip6_icmp.c | 24 +---
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index 298a48dd25..d0f5cc1456 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -143,17 +143,10 @@ void ndp_send_ra(Slirp *slirp)
 /* Build IPv6 packet */
 struct mbuf *t = m_get(slirp);
 struct ip6 *rip = mtod(t, struct ip6 *);
+size_t pl_size = 0;
 rip->ip_src = (struct in6_addr)LINKLOCAL_ADDR;
 rip->ip_dst = (struct in6_addr)ALLNODES_MULTICAST;
 rip->ip_nh = IPPROTO_ICMPV6;
-rip->ip_pl = htons(ICMP6_NDP_RA_MINLEN
-+ NDPOPT_LINKLAYER_LEN
-+ NDPOPT_PREFIXINFO_LEN
-#ifndef _WIN32
-+ NDPOPT_RDNSS_LEN
-#endif
-);
-t->m_len = sizeof(struct ip6) + ntohs(rip->ip_pl);
 
 /* Build ICMPv6 packet */
 t->m_data += sizeof(struct ip6);
@@ -171,6 +164,7 @@ void ndp_send_ra(Slirp *slirp)
 ricmp->icmp6_nra.reach_time = htonl(NDP_AdvReachableTime);
 ricmp->icmp6_nra.retrans_time = htonl(NDP_AdvRetransTime);
 t->m_data += ICMP6_NDP_RA_MINLEN;
+pl_size += ICMP6_NDP_RA_MINLEN;
 
 /* Source link-layer address (NDP option) */
 struct ndpopt *opt = mtod(t, struct ndpopt *);
@@ -178,6 +172,7 @@ void ndp_send_ra(Slirp *slirp)
 opt->ndpopt_len = NDPOPT_LINKLAYER_LEN / 8;
 in6_compute_ethaddr(rip->ip_src, opt->ndpopt_linklayer);
 t->m_data += NDPOPT_LINKLAYER_LEN;
+pl_size += NDPOPT_LINKLAYER_LEN;
 
 /* Prefix information (NDP option) */
 struct ndpopt *opt2 = mtod(t, struct ndpopt *);
@@ -192,6 +187,7 @@ void ndp_send_ra(Slirp *slirp)
 opt2->ndpopt_prefixinfo.reserved2 = 0;
 opt2->ndpopt_prefixinfo.prefix = slirp->vprefix_addr6;
 t->m_data += NDPOPT_PREFIXINFO_LEN;
+pl_size += NDPOPT_PREFIXINFO_LEN;
 
 #ifndef _WIN32
 /* Prefix information (NDP option) */
@@ -203,16 +199,14 @@ void ndp_send_ra(Slirp *slirp)
 opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
 opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
 t->m_data += NDPOPT_RDNSS_LEN;
+pl_size += NDPOPT_RDNSS_LEN;
 #endif
 
+rip->ip_pl = htons(pl_size);
+t->m_data -= sizeof(struct ip6) + pl_size;
+t->m_len = sizeof(struct ip6) + pl_size;
+
 /* ICMPv6 Checksum */
-#ifndef _WIN32
-t->m_data -= NDPOPT_RDNSS_LEN;
-#endif
-t->m_data -= NDPOPT_PREFIXINFO_LEN;
-t->m_data -= NDPOPT_LINKLAYER_LEN;
-t->m_data -= ICMP6_NDP_RA_MINLEN;
-t->m_data -= sizeof(struct ip6);
 ricmp->icmp6_cksum = ip6_cksum(t);
 
 ip6_output(NULL, t, 0);
-- 
2.11.0

[Qemu-devel] [PATCH 0/2] Fix spurious RDNSS announce

[Samuel Thibault]

Hello,

These two patches fix sending the IPv6 RDNSS announce only when
it can actually work, fixing the bug reported by Cole Robinson
<crobi...@redhat.com>.  Could somebody review them before I can send
a pull update?

Samuel Thibault (2):
  slirp: Make RA build more flexible
  slirp: Send RDNSS in RA only if host has an IPv6 DNS server

 slirp/ip6_icmp.c | 48 +++-
 1 file changed, 23 insertions(+), 25 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 2/2] slirp: Send RDNSS in RA only if host has an IPv6 DNS server

[Samuel Thibault]

Previously we would always send an RDNSS option in the RA, making the guest
try to resolve DNS through IPv6, even if the host does not actually have
and IPv6 DNS server available.

This makes the RDNSS option enabled only when an IPv6 DNS server is
available.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/ip6_icmp.c | 26 +++---
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index d0f5cc1456..00183e5945 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -189,18 +189,22 @@ void ndp_send_ra(Slirp *slirp)
 t->m_data += NDPOPT_PREFIXINFO_LEN;
 pl_size += NDPOPT_PREFIXINFO_LEN;
 
-#ifndef _WIN32
 /* Prefix information (NDP option) */
-/* disabled for windows for now, until get_dns6_addr is implemented */
-struct ndpopt *opt3 = mtod(t, struct ndpopt *);
-opt3->ndpopt_type = NDPOPT_RDNSS;
-opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
-opt3->ndpopt_rdnss.reserved = 0;
-opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
-opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
-t->m_data += NDPOPT_RDNSS_LEN;
-pl_size += NDPOPT_RDNSS_LEN;
-#endif
+{
+struct in6_addr addr;
+uint32_t scope_id;
+if (get_dns6_addr(, _id) >= 0) {
+/* Host system does have an IPv6 DNS server, announce our proxy.  
*/
+struct ndpopt *opt3 = mtod(t, struct ndpopt *);
+opt3->ndpopt_type = NDPOPT_RDNSS;
+opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
+opt3->ndpopt_rdnss.reserved = 0;
+opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
+opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
+t->m_data += NDPOPT_RDNSS_LEN;
+pl_size += NDPOPT_RDNSS_LEN;
+}
+}
 
 rip->ip_pl = htons(pl_size);
 t->m_data -= sizeof(struct ip6) + pl_size;
-- 
2.11.0

Re: [Qemu-devel] slirp + ipxe + ipv6 dns issue

[Samuel Thibault]

Hello,

Cole Robinson, on ven. 24 mars 2017 21:17:43 -0400, wrote:
> I bisected to this commit:
> 
> slirp: Add RDNSS advertisement

Mmm, I see.  Could you try the attached patch to confirm that it fixes
the issue for you too?

Samuel
diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index 298a48dd25..00183e5945 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -143,17 +143,10 @@ void ndp_send_ra(Slirp *slirp)
 /* Build IPv6 packet */
 struct mbuf *t = m_get(slirp);
 struct ip6 *rip = mtod(t, struct ip6 *);
+size_t pl_size = 0;
 rip->ip_src = (struct in6_addr)LINKLOCAL_ADDR;
 rip->ip_dst = (struct in6_addr)ALLNODES_MULTICAST;
 rip->ip_nh = IPPROTO_ICMPV6;
-rip->ip_pl = htons(ICMP6_NDP_RA_MINLEN
-+ NDPOPT_LINKLAYER_LEN
-+ NDPOPT_PREFIXINFO_LEN
-#ifndef _WIN32
-+ NDPOPT_RDNSS_LEN
-#endif
-);
-t->m_len = sizeof(struct ip6) + ntohs(rip->ip_pl);
 
 /* Build ICMPv6 packet */
 t->m_data += sizeof(struct ip6);
@@ -171,6 +164,7 @@ void ndp_send_ra(Slirp *slirp)
 ricmp->icmp6_nra.reach_time = htonl(NDP_AdvReachableTime);
 ricmp->icmp6_nra.retrans_time = htonl(NDP_AdvRetransTime);
 t->m_data += ICMP6_NDP_RA_MINLEN;
+pl_size += ICMP6_NDP_RA_MINLEN;
 
 /* Source link-layer address (NDP option) */
 struct ndpopt *opt = mtod(t, struct ndpopt *);
@@ -178,6 +172,7 @@ void ndp_send_ra(Slirp *slirp)
 opt->ndpopt_len = NDPOPT_LINKLAYER_LEN / 8;
 in6_compute_ethaddr(rip->ip_src, opt->ndpopt_linklayer);
 t->m_data += NDPOPT_LINKLAYER_LEN;
+pl_size += NDPOPT_LINKLAYER_LEN;
 
 /* Prefix information (NDP option) */
 struct ndpopt *opt2 = mtod(t, struct ndpopt *);
@@ -192,27 +187,30 @@ void ndp_send_ra(Slirp *slirp)
 opt2->ndpopt_prefixinfo.reserved2 = 0;
 opt2->ndpopt_prefixinfo.prefix = slirp->vprefix_addr6;
 t->m_data += NDPOPT_PREFIXINFO_LEN;
+pl_size += NDPOPT_PREFIXINFO_LEN;
 
-#ifndef _WIN32
 /* Prefix information (NDP option) */
-/* disabled for windows for now, until get_dns6_addr is implemented */
-struct ndpopt *opt3 = mtod(t, struct ndpopt *);
-opt3->ndpopt_type = NDPOPT_RDNSS;
-opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
-opt3->ndpopt_rdnss.reserved = 0;
-opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
-opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
-t->m_data += NDPOPT_RDNSS_LEN;
-#endif
+{
+struct in6_addr addr;
+uint32_t scope_id;
+if (get_dns6_addr(, _id) >= 0) {
+/* Host system does have an IPv6 DNS server, announce our proxy.  
*/
+struct ndpopt *opt3 = mtod(t, struct ndpopt *);
+opt3->ndpopt_type = NDPOPT_RDNSS;
+opt3->ndpopt_len = NDPOPT_RDNSS_LEN / 8;
+opt3->ndpopt_rdnss.reserved = 0;
+opt3->ndpopt_rdnss.lifetime = htonl(2 * NDP_MaxRtrAdvInterval);
+opt3->ndpopt_rdnss.addr = slirp->vnameserver_addr6;
+t->m_data += NDPOPT_RDNSS_LEN;
+pl_size += NDPOPT_RDNSS_LEN;
+}
+}
+
+rip->ip_pl = htons(pl_size);
+t->m_data -= sizeof(struct ip6) + pl_size;
+t->m_len = sizeof(struct ip6) + pl_size;
 
 /* ICMPv6 Checksum */
-#ifndef _WIN32
-t->m_data -= NDPOPT_RDNSS_LEN;
-#endif
-t->m_data -= NDPOPT_PREFIXINFO_LEN;
-t->m_data -= NDPOPT_LINKLAYER_LEN;
-t->m_data -= ICMP6_NDP_RA_MINLEN;
-t->m_data -= sizeof(struct ip6);
 ricmp->icmp6_cksum = ip6_cksum(t);
 
 ip6_output(NULL, t, 0);

Re: [Qemu-devel] [PATCH] slirp: tftp, copy sockaddr_size

[Samuel Thibault]

Marc-André Lureau, on jeu. 23 mars 2017 15:31:56 +0400, wrote:
> ASAN detects an "unknown-crash" when running pxe-test:
> 
> /ppc64/pxe/spapr-vlan: 
> =
> ==7143==ERROR: AddressSanitizer: unknown-crash on address 0x7f6dcd298d30 at 
> pc 0x55e22218830d bp 0x7f6dcd2989e0 sp 0x7f6dcd2989d0
> READ of size 128 at 0x7f6dcd298d30 thread T2
> #0 0x55e22218830c in tftp_session_allocate 
> /home/elmarco/src/qq/slirp/tftp.c:73
> #1 0x55e22218a1f8 in tftp_handle_rrq /home/elmarco/src/qq/slirp/tftp.c:289
> #2 0x55e22218b54c in tftp_input /home/elmarco/src/qq/slirp/tftp.c:446
> #3 0x55e2221833fe in udp6_input /home/elmarco/src/qq/slirp/udp6.c:82
> #4 0x55e222137b17 in ip6_input /home/elmarco/src/qq/slirp/ip6_input.c:67
> 
> Address 0x7f6dcd298d30 is located in stack of thread T2 at offset 96 in frame
> #0 0x55e222182420 in udp6_input /home/elmarco/src/qq/slirp/udp6.c:13
> 
>   This frame has 3 object(s):
> [32, 48) ''
> [96, 124) 'lhost' <== Memory access at offset 96 partially overflows this 
> variable
> [160, 200) 'save_ip' <== Memory access at offset 96 partially underflows 
> this variable
> 
> The sockaddr_storage pointer is the sockaddr_in6 lhost on the
> stack. Copy only the source addr size.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>

Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

> ---
>  slirp/tftp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index 50e714807d..a9bc4bb1b6 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -70,7 +70,7 @@ static int tftp_session_allocate(Slirp *slirp, struct 
> sockaddr_storage *srcsas,
>  
>   found:
>memset(spt, 0, sizeof(*spt));
> -  spt->client_addr = *srcsas;
> +  memcpy(>client_addr, srcsas, sockaddr_size(srcsas));
>spt->fd = -1;
>spt->block_size = 512;
>spt->client_port = tp->udp.uh_sport;
> -- 
> 2.12.0.191.gc5d8de91d
> 

-- 
Samuel
gawk; talk; nice; date; wine; grep; touch; unzip; strip;  \
touch; gasp; finger; gasp; lyx; gasp; latex; mount; fsck; \
more; yes; gasp; umount; make clean; make mrproper; sleep

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:40:19 +, wrote:
> * Samuel Thibault (samuel.thiba...@gnu.org) wrote:
> > Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:09:26 +, wrote:
> > > but only for Linux.
> > 
> > That's what I was referring to.
> 
> Yes I think that's OK because:
>a) The alternative breaks all backwards migration
>b) This doesn't casuse any problem for forward migration
> 
> There is another way, we could add a subsection that's sent whenever
> we use non-IPv4; that would (probably) fail noisily on a backwards
> migration to an old setup.  But IMHO it's not worth it in this case
> given how rare a backwards migration of an IPv6 slirp connection on
> something other than Linux is.

Alright.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:09:26 +, wrote:
> but only for Linux.

That's what I was referring to.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:00:17 +, wrote:
> * Samuel Thibault (samuel.thiba...@gnu.org) wrote:
> > Dr. David Alan Gilbert, on mar. 28 févr. 2017 16:54:46 +, wrote:
> > > I'm thinking one way to do it without changing the version would
> > > be to use the existing value for IPv4, and on reading allow any other
> > > value for IPv6 (or just the ones we know about); that would make
> > > it inwards migration compatible.
> > 
> > Right. I don't know if that's enough for QEMU requirements.
> 
> If you change the version number you break backwards migration anyway;
> but doing what I suggested would keep backwards working in most cases.

Sure, but in some cases we're breaking upward compatibility *silently*.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 16:54:46 +, wrote:
> I'm thinking one way to do it without changing the version would
> be to use the existing value for IPv4, and on reading allow any other
> value for IPv6 (or just the ones we know about); that would make
> it inwards migration compatible.

Right. I don't know if that's enough for QEMU requirements.

Samuel

Re: [Qemu-devel] [PATCH] blk: Add discard=sparse mode

[Samuel Thibault]

Hello,

Max Reitz, on lun. 27 févr. 2017 17:12:47 +0100, wrote:
> >  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> > -if (s->has_discard && s->has_fallocate) {
> > +if (s->has_discard && (s->has_fallocate || open_flags & 
> > BDRV_O_SPARSE)) {
> 
> s->has_fallocate has a meaning. I wouldn't try to call do_fallocate() if
> s->has_fallocate is false.

Ah, sorry, I didn't realize that that test wasn't only to check that
we'll be able to call fallocate(0) further down.

> Therefore, I consider this to effectively be a no-op.

Yes.

> > @@ -1098,7 +1102,8 @@ static ssize_t 
> > handle_aiocb_write_zeroes(RawPosixAIOData *aiocb)
> >  #endif
> >  
> >  #ifdef CONFIG_FALLOCATE
> > -if (s->has_fallocate && aiocb->aio_offset >= 
> > bdrv_getlength(aiocb->bs)) {
> > +if (s->has_fallocate && !(open_flags & BDRV_O_SPARSE)
> > +&& aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
> 
> First, this part is only invoked if everything before it has failed.

I misread the code indeed.

> Unless I'm mistaken, unmap/trim requests from the guest should result in
> a discard request in the block layer. This should always trigger
> handle_aiocb_discard() here and that should do what you want it to.

Mmm, indeed.  I guess I got lost in the hairy block code.

> Could you maybe give me the configuration that results in the issue
> you're describing in the commit message?

Actually I can't reproduce the issue any more.  I'm now wondering how I
ended up there.

Anyway, I'm really sorry for the noise, and thanks for the good work :)

Samuel

[Qemu-devel] [PATCH] blk: Add discard=sparse mode

[Samuel Thibault]

By default, on discard requests, the posix block backend punches holes but
re-fallocates them to keep the allocated size intact. In some situations
it is however convenient, when using sparse disk images, to see disk image
sizes shrink on discard requests.

This commit adds a discard=sparse mode which does this, by disabling the
fallocate call.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

diff --git a/block.c b/block.c
index b663204f3f..e9cd83210a 100644
--- a/block.c
+++ b/block.c
@@ -665,12 +665,14 @@ static void bdrv_join_options(BlockDriverState *bs, QDict 
*options,
  */
 int bdrv_parse_discard_flags(const char *mode, int *flags)
 {
-*flags &= ~BDRV_O_UNMAP;
+*flags &= ~(BDRV_O_UNMAP | BDRV_O_SPARSE);
 
 if (!strcmp(mode, "off") || !strcmp(mode, "ignore")) {
 /* do nothing */
 } else if (!strcmp(mode, "on") || !strcmp(mode, "unmap")) {
 *flags |= BDRV_O_UNMAP;
+} else if (!strcmp(mode, "sparse")) {
+*flags |= BDRV_O_UNMAP | BDRV_O_SPARSE;
 } else {
 return -1;
 }
diff --git a/block/file-posix.c b/block/file-posix.c
index 4de1abd023..f9efadc5be 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1057,6 +1057,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 BDRVRawState *s = aiocb->bs->opaque;
 #endif
 
+#if defined(CONFIG_FALLOCATE_PUNCH_HOLE) || defined(CONFIG_FALLOCATE)
+int open_flags = aiocb->bs->open_flags;
+#endif
+
 if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
 return handle_aiocb_write_zeroes_block(aiocb);
 }
@@ -1079,7 +1083,7 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 #endif
 
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
-if (s->has_discard && s->has_fallocate) {
+if (s->has_discard && (s->has_fallocate || open_flags & BDRV_O_SPARSE)) {
 int ret = do_fallocate(s->fd,
FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
aiocb->aio_offset, aiocb->aio_nbytes);
@@ -1098,7 +1102,8 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 #endif
 
 #ifdef CONFIG_FALLOCATE
-if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
+if (s->has_fallocate && !(open_flags & BDRV_O_SPARSE)
+&& aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
 int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
 if (ret == 0 || ret != -ENOTSUP) {
 return ret;
diff --git a/include/block/block.h b/include/block/block.h
index bde5ebda18..103313bee0 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -97,6 +97,8 @@ typedef struct HDGeometry {
   select an appropriate protocol driver,
   ignoring the format layer */
 #define BDRV_O_NO_IO   0x1 /* don't initialize for I/O */
+#define BDRV_O_SPARSE  0x2 /* make guest UNMAP/TRIM operations make 
image
+  possibly sparse */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9a84e81eed..8df4f58ddc 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -63,8 +63,8 @@ and @samp{native} (Linux only).
 @item --discard=@var{discard}
 Control whether @dfn{discard} (also known as @dfn{trim} or @dfn{unmap})
 requests are ignored or passed to the filesystem.  @var{discard} is one of
-@samp{ignore} (or @samp{off}), @samp{unmap} (or @samp{on}).  The default is
-@samp{ignore}.
+@samp{ignore} (or @samp{off}), @samp{unmap} (or @samp{on}), or @samp{sparse}.
+The default is @samp{ignore}.
 @item --detect-zeroes=@var{detect-zeroes}
 Control the automatic conversion of plain zero writes by the OS to
 driver-specific optimized zero write commands.  @var{detect-zeroes} is one of
diff --git a/qemu-options.hx b/qemu-options.hx
index bf458f83c3..f5c7455fd1 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -539,7 +539,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
 "   [,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
 "   
[,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
 "   [,readonly=on|off][,copy-on-read=on|off]\n"
-"   [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
+"   [,discard=ignore|unmap|sparse][,detect-zeroes=on|off|unmap]\n"
 "   [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
 "   [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
 "   [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
@@ -582,7 +582,7 @@ These options have the same definition as they have in 
@option{-hdachs}.
 @item aio=@var{aio}
 @var{aio} is "threads"

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Samuel Thibault, on dim. 26 févr. 2017 21:34:27 +0100, wrote:
> since we'll want to change the size of the field

Ah, no, sorry, it was forced to be 16bit, so at least the size is fine.

But I guess we don't want to change the values to have cross-OS
compatibility without changing the version.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Hello,

Dr. David Alan Gilbert, on jeu. 23 févr. 2017 11:40:45 +, wrote:
> * Daniel P. Berrange (berra...@redhat.com) wrote:
> > IOW if we transmit this data on the wire, we've effectively said that
> > our migration data format is *not* portable across different host OS
> > platforms. At that point, sending different sized types on BSD vs
> > Linux is no big deal since we're already incompatible semantically.
> 
> This is a relatively recent error; it comes from eae303ff which added
> ss_family to allow IPv6.
> 
> I suspect the right fix here is to populate a temporary for ss_family
> on the wire; if we make it conveniently match Linux's AF_INET6 values
> it might work.

So this issue is actually not new, so I asked to pull your patch series,
thanks!

Now we should indeed fix the issue. Making the values match Linux'
AF_INET6, why not (better have some known values than arbitrary values),
but I don't think we'd want to rely on this: we'll have to introduce a
versioned difference, since we'll want to change the size of the field
as well as its value on other OSes (and I don't think we want to manage
different versioning on different OSes).

Samuel

[Qemu-devel] [PULL 1/5] slirp: VMState conversion; tcpcb

[Samuel Thibault]

From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>

Convert the migration of the struct tcpcb to use a VMStateDescription,
the rest of it will come later.

Mostly mechanical, except for conversion of some 'char' to uint8_t
to ensure portability.

Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Reviewed-by: Juan Quintela <quint...@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/slirp.c   | 149 
 slirp/tcp_var.h |   6 +--
 2 files changed, 57 insertions(+), 98 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 60539de7a3..276d8cb486 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1129,53 +1129,62 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr 
guest_addr, int guest_port,
 tcp_output(sototcpcb(so));
 }
 
-static void slirp_tcp_save(QEMUFile *f, struct tcpcb *tp)
+static int slirp_tcp_post_load(void *opaque, int version)
 {
-int i;
+tcp_template((struct tcpcb *)opaque);
 
-qemu_put_sbe16(f, tp->t_state);
-for (i = 0; i < TCPT_NTIMERS; i++)
-qemu_put_sbe16(f, tp->t_timer[i]);
-qemu_put_sbe16(f, tp->t_rxtshift);
-qemu_put_sbe16(f, tp->t_rxtcur);
-qemu_put_sbe16(f, tp->t_dupacks);
-qemu_put_be16(f, tp->t_maxseg);
-qemu_put_sbyte(f, tp->t_force);
-qemu_put_be16(f, tp->t_flags);
-qemu_put_be32(f, tp->snd_una);
-qemu_put_be32(f, tp->snd_nxt);
-qemu_put_be32(f, tp->snd_up);
-qemu_put_be32(f, tp->snd_wl1);
-qemu_put_be32(f, tp->snd_wl2);
-qemu_put_be32(f, tp->iss);
-qemu_put_be32(f, tp->snd_wnd);
-qemu_put_be32(f, tp->rcv_wnd);
-qemu_put_be32(f, tp->rcv_nxt);
-qemu_put_be32(f, tp->rcv_up);
-qemu_put_be32(f, tp->irs);
-qemu_put_be32(f, tp->rcv_adv);
-qemu_put_be32(f, tp->snd_max);
-qemu_put_be32(f, tp->snd_cwnd);
-qemu_put_be32(f, tp->snd_ssthresh);
-qemu_put_sbe16(f, tp->t_idle);
-qemu_put_sbe16(f, tp->t_rtt);
-qemu_put_be32(f, tp->t_rtseq);
-qemu_put_sbe16(f, tp->t_srtt);
-qemu_put_sbe16(f, tp->t_rttvar);
-qemu_put_be16(f, tp->t_rttmin);
-qemu_put_be32(f, tp->max_sndwnd);
-qemu_put_byte(f, tp->t_oobflags);
-qemu_put_byte(f, tp->t_iobc);
-qemu_put_sbe16(f, tp->t_softerror);
-qemu_put_byte(f, tp->snd_scale);
-qemu_put_byte(f, tp->rcv_scale);
-qemu_put_byte(f, tp->request_r_scale);
-qemu_put_byte(f, tp->requested_s_scale);
-qemu_put_be32(f, tp->ts_recent);
-qemu_put_be32(f, tp->ts_recent_age);
-qemu_put_be32(f, tp->last_ack_sent);
+return 0;
 }
 
+static const VMStateDescription vmstate_slirp_tcp = {
+.name = "slirp-tcp",
+.version_id = 0,
+.post_load = slirp_tcp_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_INT16(t_state, struct tcpcb),
+VMSTATE_INT16_ARRAY(t_timer, struct tcpcb, TCPT_NTIMERS),
+VMSTATE_INT16(t_rxtshift, struct tcpcb),
+VMSTATE_INT16(t_rxtcur, struct tcpcb),
+VMSTATE_INT16(t_dupacks, struct tcpcb),
+VMSTATE_UINT16(t_maxseg, struct tcpcb),
+VMSTATE_UINT8(t_force, struct tcpcb),
+VMSTATE_UINT16(t_flags, struct tcpcb),
+VMSTATE_UINT32(snd_una, struct tcpcb),
+VMSTATE_UINT32(snd_nxt, struct tcpcb),
+VMSTATE_UINT32(snd_up, struct tcpcb),
+VMSTATE_UINT32(snd_wl1, struct tcpcb),
+VMSTATE_UINT32(snd_wl2, struct tcpcb),
+VMSTATE_UINT32(iss, struct tcpcb),
+VMSTATE_UINT32(snd_wnd, struct tcpcb),
+VMSTATE_UINT32(rcv_wnd, struct tcpcb),
+VMSTATE_UINT32(rcv_nxt, struct tcpcb),
+VMSTATE_UINT32(rcv_up, struct tcpcb),
+VMSTATE_UINT32(irs, struct tcpcb),
+VMSTATE_UINT32(rcv_adv, struct tcpcb),
+VMSTATE_UINT32(snd_max, struct tcpcb),
+VMSTATE_UINT32(snd_cwnd, struct tcpcb),
+VMSTATE_UINT32(snd_ssthresh, struct tcpcb),
+VMSTATE_INT16(t_idle, struct tcpcb),
+VMSTATE_INT16(t_rtt, struct tcpcb),
+VMSTATE_UINT32(t_rtseq, struct tcpcb),
+VMSTATE_INT16(t_srtt, struct tcpcb),
+VMSTATE_INT16(t_rttvar, struct tcpcb),
+VMSTATE_UINT16(t_rttmin, struct tcpcb),
+VMSTATE_UINT32(max_sndwnd, struct tcpcb),
+VMSTATE_UINT8(t_oobflags, struct tcpcb),
+VMSTATE_UINT8(t_iobc, struct tcpcb),
+VMSTATE_INT16(t_softerror, struct tcpcb),
+VMSTATE_UINT8(snd_scale, struct tcpcb),
+VMSTATE_UINT8(rcv_scale, struct tcpcb),
+VMSTATE_UINT8(request_r_scale, struct tcpcb),
+VMSTATE_UINT8(requested_s_scale, struct tcpcb),
+VMSTATE_UINT32(ts_recent, struct tcpcb),
+VMSTATE_UINT32(ts_recent_age, struct tcpcb),
+VMSTATE_UINT32(last_ack_sent, struct tcpcb),
+VMSTATE_END_OF_L

[Qemu-devel] [PULL 5/5] slirp: VMStatify remaining except for loop

[Samuel Thibault]

From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>

This converts the remaining components, except for the top level
loop, to VMState.

Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: Juan Quintela <quint...@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/slirp.c | 48 +++-
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 6583de8769..f8ee7f9d95 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1343,15 +1343,25 @@ static const VMStateDescription vmstate_slirp_socket = {
 }
 };
 
-static void slirp_bootp_save(QEMUFile *f, Slirp *slirp)
-{
-int i;
+static const VMStateDescription vmstate_slirp_bootp_client = {
+.name = "slirp_bootpclient",
+.fields = (VMStateField[]) {
+VMSTATE_UINT16(allocated, BOOTPClient),
+VMSTATE_BUFFER(macaddr, BOOTPClient),
+VMSTATE_END_OF_LIST()
+}
+};
 
-for (i = 0; i < NB_BOOTP_CLIENTS; i++) {
-qemu_put_be16(f, slirp->bootp_clients[i].allocated);
-qemu_put_buffer(f, slirp->bootp_clients[i].macaddr, 6);
+static const VMStateDescription vmstate_slirp = {
+.name = "slirp",
+.version_id = 4,
+.fields = (VMStateField[]) {
+VMSTATE_UINT16_V(ip_id, Slirp, 2),
+VMSTATE_STRUCT_ARRAY(bootp_clients, Slirp, NB_BOOTP_CLIENTS, 3,
+ vmstate_slirp_bootp_client, BOOTPClient),
+VMSTATE_END_OF_LIST()
 }
-}
+};
 
 static void slirp_state_save(QEMUFile *f, void *opaque)
 {
@@ -1371,22 +1381,10 @@ static void slirp_state_save(QEMUFile *f, void *opaque)
 }
 qemu_put_byte(f, 0);
 
-qemu_put_be16(f, slirp->ip_id);
-
-slirp_bootp_save(f, slirp);
+vmstate_save_state(f, _slirp, slirp, NULL);
 }
 
 
-static void slirp_bootp_load(QEMUFile *f, Slirp *slirp)
-{
-int i;
-
-for (i = 0; i < NB_BOOTP_CLIENTS; i++) {
-slirp->bootp_clients[i].allocated = qemu_get_be16(f);
-qemu_get_buffer(f, slirp->bootp_clients[i].macaddr, 6);
-}
-}
-
 static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)
 {
 Slirp *slirp = opaque;
@@ -1421,13 +1419,5 @@ static int slirp_state_load(QEMUFile *f, void *opaque, 
int version_id)
 so->extra = (void *)ex_ptr->ex_exec;
 }
 
-if (version_id >= 2) {
-slirp->ip_id = qemu_get_be16(f);
-}
-
-if (version_id >= 3) {
-slirp_bootp_load(f, slirp);
-}
-
-return 0;
+return vmstate_load_state(f, _slirp, slirp, version_id);
 }
-- 
2.11.0

[Qemu-devel] [PULL 4/5] slirp: VMStatify socket level

[Samuel Thibault]

From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>

Working up the stack, this replaces the slirp_socket_load/save
with VMState definitions.

A place holder for IPv6 support is added as a comment; it needs
testing once the rest of the IPv6 code is there.

Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: Juan Quintela <quint...@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/slirp.c  | 170 ++---
 slirp/socket.h |   6 +-
 2 files changed, 93 insertions(+), 83 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 178c2b6d14..6583de8769 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1250,40 +1250,99 @@ static const VMStateDescription vmstate_slirp_sbuf = {
 }
 };
 
+static bool slirp_older_than_v4(void *opaque, int version_id)
+{
+return version_id < 4;
+}
 
-static void slirp_socket_save(QEMUFile *f, struct socket *so)
+static bool slirp_family_inet(void *opaque, int version_id)
 {
-qemu_put_be32(f, so->so_urgc);
-qemu_put_be16(f, so->so_ffamily);
-switch (so->so_ffamily) {
-case AF_INET:
-qemu_put_be32(f, so->so_faddr.s_addr);
-qemu_put_be16(f, so->so_fport);
-break;
-default:
-error_report("so_ffamily unknown, unable to save so_faddr and"
- " so_fport");
-}
-qemu_put_be16(f, so->so_lfamily);
-switch (so->so_lfamily) {
-case AF_INET:
-qemu_put_be32(f, so->so_laddr.s_addr);
-qemu_put_be16(f, so->so_lport);
-break;
-default:
-error_report("so_ffamily unknown, unable to save so_laddr and"
- " so_lport");
+union slirp_sockaddr *ssa = (union slirp_sockaddr *)opaque;
+return ssa->ss.ss_family == AF_INET;
+}
+
+static int slirp_socket_pre_load(void *opaque)
+{
+struct socket *so = opaque;
+if (tcp_attach(so) < 0) {
+return -ENOMEM;
 }
-qemu_put_byte(f, so->so_iptos);
-qemu_put_byte(f, so->so_emu);
-qemu_put_byte(f, so->so_type);
-qemu_put_be32(f, so->so_state);
-/* TODO: Build vmstate at this level */
-vmstate_save_state(f, _slirp_sbuf, >so_rcv, 0);
-vmstate_save_state(f, _slirp_sbuf, >so_snd, 0);
-vmstate_save_state(f, _slirp_tcp, so->so_tcpcb, 0);
+/* Older versions don't load these fields */
+so->so_ffamily = AF_INET;
+so->so_lfamily = AF_INET;
+return 0;
 }
 
+#ifndef _WIN32
+#define VMSTATE_SS_FAMILY(f, s) VMSTATE_UINT16(f, s)
+#define VMSTATE_SIN4_ADDR(f, s, t) VMSTATE_UINT32_TEST(f, s, t)
+#else
+/* Win has a signed family number */
+#define VMSTATE_SS_FAMILY(f, s) VMSTATE_INT16(f, s)
+/* Win uses u_long rather than uint32_t - but it's still 32bits long */
+#define VMSTATE_SIN4_ADDR(f, s, t) VMSTATE_SINGLE_TEST(f, s, t, 0, \
+   vmstate_info_uint32, u_long)
+#endif
+
+static const VMStateDescription vmstate_slirp_socket_addr = {
+.name = "slirp-socket-addr",
+.version_id = 4,
+.fields = (VMStateField[]) {
+VMSTATE_SS_FAMILY(ss.ss_family, union slirp_sockaddr),
+VMSTATE_SIN4_ADDR(sin.sin_addr.s_addr, union slirp_sockaddr,
+slirp_family_inet),
+VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
+slirp_family_inet),
+
+#if 0
+/* Untested: Needs checking by someone with IPv6 test */
+VMSTATE_BUFFER_TEST(sin6.sin6_addr, union slirp_sockaddr,
+slirp_family_inet6),
+VMSTATE_UINT16_TEST(sin6.sin6_port, union slirp_sockaddr,
+slirp_family_inet6),
+VMSTATE_UINT32_TEST(sin6.sin6_flowinfo, union slirp_sockaddr,
+slirp_family_inet6),
+VMSTATE_UINT32_TEST(sin6.sin6_scope_id, union slirp_sockaddr,
+slirp_family_inet6),
+#endif
+
+VMSTATE_END_OF_LIST()
+}
+};
+
+static const VMStateDescription vmstate_slirp_socket = {
+.name = "slirp-socket",
+.version_id = 4,
+.pre_load = slirp_socket_pre_load,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(so_urgc, struct socket),
+/* Pre-v4 versions */
+VMSTATE_SIN4_ADDR(so_faddr.s_addr, struct socket,
+slirp_older_than_v4),
+VMSTATE_SIN4_ADDR(so_laddr.s_addr, struct socket,
+slirp_older_than_v4),
+VMSTATE_UINT16_TEST(so_fport, struct socket, slirp_older_than_v4),
+VMSTATE_UINT16_TEST(so_lport, struct socket, slirp_older_than_v4),
+/* v4 and newer */
+VMSTATE_STRUCT(fhost, struct socket, 4, vmstate_slirp_socket_addr,
+   union slirp_sockaddr),
+VMSTATE_STRUCT(lhost, struct socket, 4, vmstate_slirp_socket_addr,
+ 

[Qemu-devel] [PULL 2/5] slirp: VMStatify sbuf

[Samuel Thibault]

From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>

Convert the sbuf structure to a VMStateDescription.
Note this uses the VMSTATE_WITH_TMP mechanism to calculate
and reload the offsets based on the pointers.

Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
Reviewed-by: Juan Quintela <quint...@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/sbuf.h  |   4 +-
 slirp/slirp.c | 116 ++
 2 files changed, 78 insertions(+), 42 deletions(-)

diff --git a/slirp/sbuf.h b/slirp/sbuf.h
index efcec39a6b..a722ecb629 100644
--- a/slirp/sbuf.h
+++ b/slirp/sbuf.h
@@ -12,8 +12,8 @@
 #define sbspace(sb) ((sb)->sb_datalen - (sb)->sb_cc)
 
 struct sbuf {
-   u_int   sb_cc;  /* actual chars in buffer */
-   u_int   sb_datalen; /* Length of data  */
+   uint32_t sb_cc; /* actual chars in buffer */
+   uint32_t sb_datalen;/* Length of data  */
char*sb_wptr;   /* write pointer. points to where the next
 * bytes should be written in the sbuf */
char*sb_rptr;   /* read pointer. points to where the next
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 276d8cb486..178c2b6d14 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1185,19 +1185,72 @@ static const VMStateDescription vmstate_slirp_tcp = {
 }
 };
 
-static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
+/* The sbuf has a pair of pointers that are migrated as offsets;
+ * we calculate the offsets and restore the pointers using
+ * pre_save/post_load on a tmp structure.
+ */
+struct sbuf_tmp {
+struct sbuf *parent;
+uint32_t roff, woff;
+};
+
+static void sbuf_tmp_pre_save(void *opaque)
+{
+struct sbuf_tmp *tmp = opaque;
+tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data;
+tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data;
+}
+
+static int sbuf_tmp_post_load(void *opaque, int version)
 {
-uint32_t off;
-
-qemu_put_be32(f, sbuf->sb_cc);
-qemu_put_be32(f, sbuf->sb_datalen);
-off = (uint32_t)(sbuf->sb_wptr - sbuf->sb_data);
-qemu_put_sbe32(f, off);
-off = (uint32_t)(sbuf->sb_rptr - sbuf->sb_data);
-qemu_put_sbe32(f, off);
-qemu_put_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
+struct sbuf_tmp *tmp = opaque;
+uint32_t requested_len = tmp->parent->sb_datalen;
+
+/* Allocate the buffer space used by the field after the tmp */
+sbreserve(tmp->parent, tmp->parent->sb_datalen);
+
+if (tmp->parent->sb_datalen != requested_len) {
+return -ENOMEM;
+}
+if (tmp->woff >= requested_len ||
+tmp->roff >= requested_len) {
+error_report("invalid sbuf offsets r/w=%u/%u len=%u",
+ tmp->roff, tmp->woff, requested_len);
+return -EINVAL;
+}
+
+tmp->parent->sb_wptr = tmp->parent->sb_data + tmp->woff;
+tmp->parent->sb_rptr = tmp->parent->sb_data + tmp->roff;
+
+return 0;
 }
 
+
+static const VMStateDescription vmstate_slirp_sbuf_tmp = {
+.name = "slirp-sbuf-tmp",
+.post_load = sbuf_tmp_post_load,
+.pre_save  = sbuf_tmp_pre_save,
+.version_id = 0,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(woff, struct sbuf_tmp),
+VMSTATE_UINT32(roff, struct sbuf_tmp),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static const VMStateDescription vmstate_slirp_sbuf = {
+.name = "slirp-sbuf",
+.version_id = 0,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(sb_cc, struct sbuf),
+VMSTATE_UINT32(sb_datalen, struct sbuf),
+VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp, vmstate_slirp_sbuf_tmp),
+VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, sb_datalen),
+VMSTATE_END_OF_LIST()
+}
+};
+
+
 static void slirp_socket_save(QEMUFile *f, struct socket *so)
 {
 qemu_put_be32(f, so->so_urgc);
@@ -1225,8 +1278,9 @@ static void slirp_socket_save(QEMUFile *f, struct socket 
*so)
 qemu_put_byte(f, so->so_emu);
 qemu_put_byte(f, so->so_type);
 qemu_put_be32(f, so->so_state);
-slirp_sbuf_save(f, >so_rcv);
-slirp_sbuf_save(f, >so_snd);
+/* TODO: Build vmstate at this level */
+vmstate_save_state(f, _slirp_sbuf, >so_rcv, 0);
+vmstate_save_state(f, _slirp_sbuf, >so_snd, 0);
 vmstate_save_state(f, _slirp_tcp, so->so_tcpcb, 0);
 }
 
@@ -1263,31 +1317,9 @@ static void slirp_state_save(QEMUFile *f, void *opaque)
 slirp_bootp_save(f, slirp);
 }
 
-static int slirp_sbuf_load(QEMUFile *f, struct sbuf *sbuf)
-{
-uint32_t off, sb_cc, sb_datalen;
-
-sb_cc = qemu_get_be32(f);
-sb_datalen = qemu_get_be32(f);
-
-sbreserve(sbuf, sb_da

[Qemu-devel] [PULL 3/5] slirp: Common lhost/fhost union

[Samuel Thibault]

From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>

The socket structure has a pair of unions for lhost and fhost
addresses; the unions are identical so split them out into
a separate union declaration.

Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Reviewed-by: Juan Quintela <quint...@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/socket.h | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/slirp/socket.h b/slirp/socket.h
index 8feed2aea4..c1be77eaf3 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -15,6 +15,12 @@
  * Our socket structure
  */
 
+union slirp_sockaddr {
+struct sockaddr_storage ss;
+struct sockaddr_in sin;
+struct sockaddr_in6 sin6;
+};
+
 struct socket {
   struct socket *so_next,*so_prev;  /* For a linked list of sockets */
 
@@ -31,22 +37,14 @@ struct socket {
   struct tcpiphdr *so_ti; /* Pointer to the original ti within
* so_mconn, for non-blocking connections */
   int so_urgc;
-  union {   /* foreign host */
-  struct sockaddr_storage ss;
-  struct sockaddr_in sin;
-  struct sockaddr_in6 sin6;
-  } fhost;
+  union slirp_sockaddr fhost;  /* Foreign host */
 #define so_faddr fhost.sin.sin_addr
 #define so_fport fhost.sin.sin_port
 #define so_faddr6 fhost.sin6.sin6_addr
 #define so_fport6 fhost.sin6.sin6_port
 #define so_ffamily fhost.ss.ss_family
 
-  union {   /* local host */
-  struct sockaddr_storage ss;
-  struct sockaddr_in sin;
-  struct sockaddr_in6 sin6;
-  } lhost;
+  union slirp_sockaddr lhost;  /* Local host */
 #define so_laddr lhost.sin.sin_addr
 #define so_lport lhost.sin.sin_port
 #define so_laddr6 lhost.sin6.sin6_addr
-- 
2.11.0

[Qemu-devel] [PULL 0/5] slirp updates

[Samuel Thibault]

The following changes since commit 685783c5b69c83c942d1fc21679311eeb8f79ab9:

  Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into 
staging (2017-02-26 16:38:40 +)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to c363a5b7f9ca9e802665587900b7ea1aefcf26ea:

  slirp: VMStatify remaining except for loop (2017-02-26 21:16:38 +0100)


slirp updates


Dr. David Alan Gilbert (5):
  slirp: VMState conversion; tcpcb
  slirp: VMStatify sbuf
  slirp: Common lhost/fhost union
  slirp: VMStatify socket level
  slirp: VMStatify remaining except for loop

 slirp/sbuf.h|   4 +-
 slirp/slirp.c   | 449 
 slirp/socket.h  |  24 ++-
 slirp/tcp_var.h |   6 +-
 4 files changed, 238 insertions(+), 245 deletions(-)

[Qemu-devel] [PULL 2/3] slirp: Convert mbufs to use g_malloc() and g_free()

[Samuel Thibault]

From: Peter Maydell <peter.mayd...@linaro.org>

The mbuf code currently doesn't check the result of doing a malloc()
or realloc() of its data (spotted by Coverity, CID 1238946).
Since the m_inc() API assumes that extending an mbuf must succeed,
just convert to g_malloc() and g_free().

Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/mbuf.c | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index 7eddc217e4..5ff24559fd 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -10,7 +10,7 @@
  * FreeBSD.  They are fixed size, determined by the MTU,
  * so that one whole packet can fit.  Mbuf's cannot be
  * chained together.  If there's more data than the mbuf
- * could hold, an external malloced buffer is pointed to
+ * could hold, an external g_malloced buffer is pointed to
  * by m_ext (and the data pointers) and M_EXT is set in
  * the flags
  */
@@ -41,26 +41,26 @@ void m_cleanup(Slirp *slirp)
 while ((struct quehead *) m != >m_usedlist) {
 next = m->m_next;
 if (m->m_flags & M_EXT) {
-free(m->m_ext);
+g_free(m->m_ext);
 }
-free(m);
+g_free(m);
 m = next;
 }
 m = (struct mbuf *) slirp->m_freelist.qh_link;
 while ((struct quehead *) m != >m_freelist) {
 next = m->m_next;
-free(m);
+g_free(m);
 m = next;
 }
 }
 
 /*
  * Get an mbuf from the free list, if there are none
- * malloc one
+ * allocate one
  *
  * Because fragmentation can occur if we alloc new mbufs and
  * free old mbufs, we mark all mbufs above mbuf_thresh as M_DOFREE,
- * which tells m_free to actually free() it
+ * which tells m_free to actually g_free() it
  */
 struct mbuf *
 m_get(Slirp *slirp)
@@ -71,8 +71,7 @@ m_get(Slirp *slirp)
DEBUG_CALL("m_get");
 
if (slirp->m_freelist.qh_link == >m_freelist) {
-   m = (struct mbuf *)malloc(SLIRP_MSIZE);
-   if (m == NULL) goto end_error;
+m = g_malloc(SLIRP_MSIZE);
slirp->mbuf_alloced++;
if (slirp->mbuf_alloced > MBUF_THRESH)
flags = M_DOFREE;
@@ -94,7 +93,6 @@ m_get(Slirp *slirp)
 m->m_prevpkt = NULL;
 m->resolution_requested = false;
 m->expiration_date = (uint64_t)-1;
-end_error:
DEBUG_ARG("m = %p", m);
return m;
 }
@@ -112,15 +110,15 @@ m_free(struct mbuf *m)
   remque(m);
 
/* If it's M_EXT, free() it */
-   if (m->m_flags & M_EXT)
-  free(m->m_ext);
-
+if (m->m_flags & M_EXT) {
+g_free(m->m_ext);
+}
/*
 * Either free() it or put it on the free list
 */
if (m->m_flags & M_DOFREE) {
m->slirp->mbuf_alloced--;
-   free(m);
+g_free(m);
} else if ((m->m_flags & M_FREELIST) == 0) {
insque(m,>slirp->m_freelist);
m->m_flags = M_FREELIST; /* Clobber other flags */
@@ -130,7 +128,7 @@ m_free(struct mbuf *m)
 
 /*
  * Copy data from one mbuf to the end of
- * the other.. if result is too big for one mbuf, malloc()
+ * the other.. if result is too big for one mbuf, allocate
  * an M_EXT data segment
  */
 void
@@ -160,12 +158,12 @@ m_inc(struct mbuf *m, int size)
 
 if (m->m_flags & M_EXT) {
  datasize = m->m_data - m->m_ext;
- m->m_ext = (char *)realloc(m->m_ext,size);
+  m->m_ext = g_realloc(m->m_ext, size);
  m->m_data = m->m_ext + datasize;
 } else {
  char *dat;
  datasize = m->m_data - m->m_dat;
- dat = (char *)malloc(size);
+  dat = g_malloc(size);
  memcpy(dat, m->m_dat, m->m_size);
 
  m->m_ext = dat;
-- 
2.11.0

[Qemu-devel] [PULL 0/3] slirp updates

[Samuel Thibault]

The following changes since commit 6528a4c1f20c1ba5a22ab84bec6788a574ac04c8:

  Merge remote-tracking branch 'remotes/cody/tags/block-pull-request' into 
staging (2017-02-26 11:47:00 +)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to bd5d2353aa69e68e45d8a89787bab17c155e9e24:

  slirp: tcp_listen(): Don't try to close() an fd we never opened (2017-02-26 
15:39:29 +0100)


slirp updates


Peter Maydell (3):
  slirp: Check qemu_socket() return value in udp_listen()
  slirp: Convert mbufs to use g_malloc() and g_free()
  slirp: tcp_listen(): Don't try to close() an fd we never opened

 slirp/mbuf.c   | 30 ++
 slirp/socket.c |  4 +++-
 slirp/udp.c|  4 
 3 files changed, 21 insertions(+), 17 deletions(-)

[Qemu-devel] [PULL 1/3] slirp: Check qemu_socket() return value in udp_listen()

[Samuel Thibault]

From: Peter Maydell <peter.mayd...@linaro.org>

Check the return value from qemu_socket() rather than trying to
pass it to bind() as an fd argument even if it's negative.
This wouldn't have caused any negative consequences, because
it won't be a valid fd number and the bind call will fail;
but Coverity complains (CID 1005723).

Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/udp.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/slirp/udp.c b/slirp/udp.c
index 93d7224792..227d779022 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -335,6 +335,10 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, 
uint32_t laddr,
return NULL;
}
so->s = qemu_socket(AF_INET,SOCK_DGRAM,0);
+if (so->s < 0) {
+sofree(so);
+return NULL;
+}
so->so_expire = curtime + SO_EXPIRE;
insque(so, >udb);
 
-- 
2.11.0

[Qemu-devel] [PULL 3/3] slirp: tcp_listen(): Don't try to close() an fd we never opened

[Samuel Thibault]

From: Peter Maydell <peter.mayd...@linaro.org>

Coverity points out (CID 1005725) that an error-exit path in tcp_listen()
will try to close(s) even if the reason it got there was that the
qemu_socket() failed and s was never opened.  Not only that, this isn't even
the right function to use, because we need closesocket() to do the right
thing on Windows.  Change to using the right function and only calling it if
needed.

Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/socket.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 6c18971368..86927722e1 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -713,7 +713,9 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, 
uint32_t laddr,
(listen(s,1) < 0)) {
int tmperrno = errno; /* Don't clobber the real reason we 
failed */
 
-   close(s);
+if (s >= 0) {
+closesocket(s);
+}
sofree(so);
/* Restore the real errno */
 #ifdef _WIN32
-- 
2.11.0

Re: [Qemu-devel] libslirp and QEMU slirp

[Samuel Thibault]

Hello,

Stefan Hajnoczi, on Mon 06 Feb 2017 11:55:05 +, wrote:
> I'm proposing this topic for discussion because there might be common
> ground for Samuel from QEMU and the VDE folks to collaborate.
> 
> Samuel: Should QEMU join forces with libslirp?

Well, there are not that many forces from the QEMU side :)

Personally, I'm fine with seeing slirp move to a separate playground
that qemu would depend on and others could contribute to.  It just needs
to be integrated to distros so that qemu can get it.

That being said, the integration of slirp and qemu is not so loose,
notably on the top of my head:

- we are using timers for icmp announcements
- qemu needs to be able to save/restore state, with compatibility with
previous versions

We'd need to see how that can be expressed as a library.

Samuel

[Qemu-devel] [PATCH] Drop duplicate display option documentation

[Samuel Thibault]

The curses and none possibilities are already documented on a separate line,
so documenting it on the sdl line was both unneeded and confusing.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

diff --git a/qemu-options.hx b/qemu-options.hx
index c534a2f7f9..9c81d3752d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -927,7 +927,7 @@ ETEXI
 
 DEF("display", HAS_ARG, QEMU_OPTION_display,
 "-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n"
-"[,window_close=on|off][,gl=on|off]|curses|none|\n"
+"[,window_close=on|off][,gl=on|off]\n"
 "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
 "-display vnc=[,]\n"
 "-display curses\n"

Re: [Qemu-devel] [PULL v2 05/11] sdl2: set window ID

[Samuel Thibault]

Hello,

Gerd Hoffmann, on Thu 12 Jan 2017 16:56:49 +0100, wrote:
> On Do, 2017-01-12 at 16:10 +0100, Stefan Weil wrote:
> > This commit breaks builds for Windows. See below for details.
> 
> > Windows does not use X11. gcc fails:
> > 
> >   CC  ui/sdl2.o
> > /qemu/ui/sdl2.c: In function ‘sdl_display_init’:
> > /qemu/ui/sdl2.c:821:54: error: ‘union ’ has no member named ‘x11’
> >  qemu_console_set_window_id(con, info.info.x11.window);
> 
> Oops.

Oops, sorry, it seems to have been overlooked indeed.  Can you easily
test the attached patch?

Samuel
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 9a79b17b92..0f41d1fa90 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -762,6 +762,9 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 char *filename;
 int i;
 SDL_SysWMinfo info;
+#if defined(SDL_VIDEO_DRIVER_WINDOWS) || defined(SDL_VIDEO_DRIVER_X11)
+int window_id;
+#endif
 
 if (no_frame) {
 gui_noframe = 1;
@@ -817,9 +820,17 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 sdl2_console[i].dcl.con = con;
 register_displaychangelistener(_console[i].dcl);
 
+#if defined(SDL_VIDEO_DRIVER_WINDOWS) || defined(SDL_VIDEO_DRIVER_X11)
 if (SDL_GetWindowWMInfo(sdl2_console[i].real_window, )) {
-qemu_console_set_window_id(con, info.info.x11.window);
+#ifdef SDL_VIDEO_DRIVER_WINDOWS
+window_id = (int)(uintptr_t) info.info.win.hwnd;
+#endif
+#ifdef SDL_VIDEO_DRIVER_X11
+window_id = info.info.x11.window;
+#endif
+qemu_console_set_window_id(con, window_id);
 }
+#endif
 }
 
 /* Load a 32x32x4 image. White pixels are transparent. */

[Qemu-devel] [PATCH 2/3] console: move window ID code from baum to sdl

[Samuel Thibault]

This moves the SDL bits for window ID from the baum driver to SDL, as
well as fixing the build for non-X11.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

---
Difference from v3: Use qemu_console_lookup_by_index and
qemu_console_is_graphic to access the console
---
 backends/baum.c | 31 ---
 ui/sdl.c| 25 +
 2 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/backends/baum.c b/backends/baum.c
index b92369d840..b045ef49c5 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -27,12 +27,10 @@
 #include "sysemu/char.h"
 #include "qemu/timer.h"
 #include "hw/usb.h"
+#include "ui/console.h"
 #include 
 #include 
 #include 
-#ifdef CONFIG_SDL
-#include 
-#endif
 
 #if 0
 #define DPRINTF(fmt, ...) \
@@ -227,12 +225,8 @@ static const uint8_t nabcc_translation[2][256] = {
 /* The guest OS has started discussing with us, finish initializing BrlAPI */
 static int baum_deferred_init(BaumDriverState *baum)
 {
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-SDL_SysWMinfo info;
-#endif
-#endif
-int tty;
+int tty = BRLAPI_TTY_DEFAULT;
+QemuConsole *con;
 
 if (baum->deferred_init) {
 return 1;
@@ -243,21 +237,12 @@ static int baum_deferred_init(BaumDriverState *baum)
 return 0;
 }
 
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-memset(, 0, sizeof(info));
-SDL_VERSION();
-if (SDL_GetWMInfo()) {
-tty = info.info.x11.wmwindow;
-} else {
-#endif
-#endif
-tty = BRLAPI_TTY_DEFAULT;
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
+con = qemu_console_lookup_by_index(0);
+if (con && qemu_console_is_graphic(con)) {
+tty = qemu_console_get_window_id(con);
+if (tty == -1)
+tty = BRLAPI_TTY_DEFAULT;
 }
-#endif
-#endif
 
 if (brlapi__enterTtyMode(baum->brlapi, tty, NULL) == -1) {
 brlapi_perror("baum: brlapi__enterTtyMode");
diff --git a/ui/sdl.c b/ui/sdl.c
index d8cf5bcf74..19e8a848a7 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -947,6 +947,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 int flags;
 uint8_t data = 0;
 const SDL_VideoInfo *vi;
+SDL_SysWMinfo info;
 char *filename;
 
 #if defined(__APPLE__)
@@ -1023,5 +1024,29 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 sdl_cursor_hidden = SDL_CreateCursor(, , 8, 1, 0, 0);
 sdl_cursor_normal = SDL_GetCursor();
 
+memset(, 0, sizeof(info));
+SDL_VERSION();
+if (SDL_GetWMInfo()) {
+int i;
+for (i = 0; ; i++) {
+/* All consoles share the same window */
+QemuConsole *con = qemu_console_lookup_by_index(i);
+if (con) {
+#if defined(SDL_VIDEO_DRIVER_X11)
+qemu_console_set_window_id(con, info.info.x11.wmwindow);
+#elif defined(SDL_VIDEO_DRIVER_NANOX) || \
+  defined(SDL_VIDEO_DRIVER_WINDIB) || defined(SDL_VIDEO_DRIVER_DDRAW) || \
+  defined(SDL_VIDEO_DRIVER_GAPI) || \
+  defined(SDL_VIDEO_DRIVER_RISCOS)
+qemu_console_set_window_id(con, (int) (uintptr_t) info.window);
+#else
+qemu_console_set_window_id(con, info.data);
+#endif
+} else {
+break;
+}
+}
+}
+
 atexit(sdl_cleanup);
 }
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] sdl2: set window ID

[Samuel Thibault]

This uses the console API to record the window ID of the SDL2 windows.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 ui/sdl2.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 30d2a3c35d..9a79b17b92 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -761,6 +761,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 uint8_t data = 0;
 char *filename;
 int i;
+SDL_SysWMinfo info;
 
 if (no_frame) {
 gui_noframe = 1;
@@ -786,6 +787,8 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 exit(1);
 }
 SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
+memset(, 0, sizeof(info));
+SDL_VERSION();
 
 for (i = 0;; i++) {
 QemuConsole *con = qemu_console_lookup_by_index(i);
@@ -813,6 +816,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 #endif
 sdl2_console[i].dcl.con = con;
 register_displaychangelistener(_console[i].dcl);
+
+if (SDL_GetWindowWMInfo(sdl2_console[i].real_window, )) {
+qemu_console_set_window_id(con, info.info.x11.window);
+}
 }
 
 /* Load a 32x32x4 image. White pixels are transparent. */
-- 
2.11.0

[Qemu-devel] [PATCH 1/3] console: add API to get underlying gui window ID

[Samuel Thibault]

This adds two console functions, qemu_console_set_window_id and
qemu_graphic_console_get_window_id, to let graphical backend record the
window id in the QemuConsole structure, and let the baum driver read it.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

---
Difference from v4: select console with a QemuConsole* parameter instead
of an integer index.
---
 include/ui/console.h |  4 
 ui/console.c | 11 +++
 2 files changed, 15 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index e2589e2134..ee8c407cac 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -394,6 +394,10 @@ uint32_t qemu_console_get_head(QemuConsole *con);
 QemuUIInfo *qemu_console_get_ui_info(QemuConsole *con);
 int qemu_console_get_width(QemuConsole *con, int fallback);
 int qemu_console_get_height(QemuConsole *con, int fallback);
+/* Return the low-level window id for the console */
+int qemu_console_get_window_id(QemuConsole *con);
+/* Set the low-level window id for the console */
+void qemu_console_set_window_id(QemuConsole *con, int window_id);
 
 void console_select(unsigned int index);
 void qemu_console_resize(QemuConsole *con, int width, int height);
diff --git a/ui/console.c b/ui/console.c
index ed888e55ea..b9575f2ee5 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -124,6 +124,7 @@ struct QemuConsole {
 int dcls;
 DisplayChangeListener *gl;
 bool gl_block;
+int window_id;
 
 /* Graphic console state.  */
 Object *device;
@@ -273,6 +274,16 @@ void graphic_hw_gl_block(QemuConsole *con, bool block)
 }
 }
 
+int qemu_console_get_window_id(QemuConsole *con)
+{
+return con->window_id;
+}
+
+void qemu_console_set_window_id(QemuConsole *con, int window_id)
+{
+con->window_id = window_id;
+}
+
 void graphic_hw_invalidate(QemuConsole *con)
 {
 if (!con) {
-- 
2.11.0

[Qemu-devel] [PATCHv4 0/3] Move getting XWindow ID from baum driver to graphical backend

[Samuel Thibault]

Hello,

This is a respin of moving getting XWindow ID from baum to actual
graphical backends.  This only contains the new API, the move
of existing code to the new API, and the addition of support for
SDL2. Gtk will need more discussion and work.

Compared to v3, this makes the API use a QemuConsole* parameter instead of an
integer index,

Samuel

Samuel Thibault (3):
  console: add API to get underlying gui window ID
  console: move window ID code from baum to sdl
  sdl2: set window ID

 backends/baum.c  | 31 ---
 include/ui/console.h |  4 
 ui/console.c | 11 +++
 ui/sdl.c | 25 +
 ui/sdl2.c|  7 +++
 5 files changed, 55 insertions(+), 23 deletions(-)

-- 
2.11.0

Re: [Qemu-devel] [PULL 0/2] slirp updates: MIN/MAX, tftp dynamic blocks

[Samuel Thibault]

no-re...@patchew.org, on Tue 20 Dec 2016 15:15:43 -0800, wrote:
> Your series seems to have some coding style problems. See output below for
> more information:


> ERROR: suspect code indent for conditional statements (10, 14)
> #90: FILE: slirp/tftp.c:393:
> +  if (blksize > 0) {
> +  spt->block_size = MIN(blksize, TFTP_BLOCKSIZE_MAX);

This is a false positive.

Samuel

Re: [Qemu-devel] [PATCH] slirp: support dynamic block size for TFTP transfers

[Samuel Thibault]

Hervé Poussineau, on Mon 21 Nov 2016 20:45:49 +0100, wrote:
> The blocksize option is defined in RFC 1783.
> We now support block sizes between 1 and 1432 bytes, instead of 512 only.
> 
> Signed-off-by: Hervé Poussineau 

I fixed a couple of trivial things and commited to my tree, thanks!

Samuel

[Qemu-devel] [PULL 1/2] slirp, disas: Replace min/max with MIN/MAX macros

[Samuel Thibault]

From: Yuval Shaia <yuval.sh...@oracle.com>

Signed-off-by: Yuval Shaia <yuval.sh...@oracle.com>
Reviewed-by: Markus Armbruster <arm...@redhat.com>
Reviewed-by: Laurent Vivier <lviv...@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 disas/m68k.c   |  8 ++--
 slirp/dhcpv6.c |  2 +-
 slirp/ip6_icmp.c   |  2 +-
 slirp/slirp.c  |  2 +-
 slirp/slirp.h  |  5 -
 slirp/tcp_input.c  | 16 
 slirp/tcp_output.c |  6 +++---
 slirp/tcp_timer.c  |  2 +-
 slirp/tcpip.h  |  2 +-
 9 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/disas/m68k.c b/disas/m68k.c
index 8e7c3f76c4..073abb9efd 100644
--- a/disas/m68k.c
+++ b/disas/m68k.c
@@ -4698,10 +4698,6 @@ get_field (const unsigned char *data, enum 
floatformat_byteorders order,
   return result;
 }
 
-#ifndef min
-#define min(a, b) ((a) < (b) ? (a) : (b))
-#endif
-
 /* Convert from FMT to a double.
FROM is the address of the extended float.
Store the double in *TO.  */
@@ -4733,7 +4729,7 @@ floatformat_to_double (const struct floatformat *fmt,
   nan = 0;
   while (mant_bits_left > 0)
{
- mant_bits = min (mant_bits_left, 32);
+  mant_bits = MIN(mant_bits_left, 32);
 
  if (get_field (ufrom, fmt->byteorder, fmt->totalsize,
 mant_off, mant_bits) != 0)
@@ -4793,7 +4789,7 @@ floatformat_to_double (const struct floatformat *fmt,
 
   while (mant_bits_left > 0)
 {
-  mant_bits = min (mant_bits_left, 32);
+  mant_bits = MIN(mant_bits_left, 32);
 
   mant = get_field (ufrom, fmt->byteorder, fmt->totalsize,
 mant_off, mant_bits);
diff --git a/slirp/dhcpv6.c b/slirp/dhcpv6.c
index 02c51c7756..d266611e85 100644
--- a/slirp/dhcpv6.c
+++ b/slirp/dhcpv6.c
@@ -168,7 +168,7 @@ static void dhcpv6_info_request(Slirp *slirp, struct 
sockaddr_in6 *srcsas,
 sa[0], sa[1], sa[2], sa[3], sa[4], sa[5], sa[6], sa[7],
 sa[8], sa[9], sa[10], sa[11], sa[12], sa[13], sa[14],
 sa[15], slirp->bootp_filename);
-slen = min(slen, smaxlen);
+slen = MIN(slen, smaxlen);
 *resp++ = slen >> 8;/* option-len high byte */
 *resp++ = slen; /* option-len low byte */
 resp += slen;
diff --git a/slirp/ip6_icmp.c b/slirp/ip6_icmp.c
index 6d18e28985..298a48dd25 100644
--- a/slirp/ip6_icmp.c
+++ b/slirp/ip6_icmp.c
@@ -95,7 +95,7 @@ void icmp6_send_error(struct mbuf *m, uint8_t type, uint8_t 
code)
 #endif
 
 rip->ip_nh = IPPROTO_ICMPV6;
-const int error_data_len = min(m->m_len,
+const int error_data_len = MIN(m->m_len,
 IF_MTU - (sizeof(struct ip6) + ICMP6_ERROR_MINLEN));
 rip->ip_pl = htons(ICMP6_ERROR_MINLEN + error_data_len);
 t->m_len = sizeof(struct ip6) + ntohs(rip->ip_pl);
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 6e2b4e5a90..60539de7a3 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -774,7 +774,7 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
 static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
 {
 struct slirp_arphdr *ah = (struct slirp_arphdr *)(pkt + ETH_HLEN);
-uint8_t arp_reply[max(ETH_HLEN + sizeof(struct slirp_arphdr), 64)];
+uint8_t arp_reply[MAX(ETH_HLEN + sizeof(struct slirp_arphdr), 64)];
 struct ethhdr *reh = (struct ethhdr *)arp_reply;
 struct slirp_arphdr *rah = (struct slirp_arphdr *)(arp_reply + ETH_HLEN);
 int ar_op;
diff --git a/slirp/slirp.h b/slirp/slirp.h
index a1f3139134..3877f667f0 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -292,9 +292,4 @@ int tcp_emu(struct socket *, struct mbuf *);
 int tcp_ctl(struct socket *);
 struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
 
-#ifndef _WIN32
-#define min(x,y) ((x) < (y) ? (x) : (y))
-#define max(x,y) ((x) > (y) ? (x) : (y))
-#endif
-
 #endif
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index c5063a918d..edb98f06f3 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -596,7 +596,7 @@ findso:
   win = sbspace(>so_rcv);
  if (win < 0)
win = 0;
- tp->rcv_wnd = max(win, (int)(tp->rcv_adv - tp->rcv_nxt));
+  tp->rcv_wnd = MAX(win, (int)(tp->rcv_adv - tp->rcv_nxt));
}
 
switch (tp->t_state) {
@@ -1065,8 +1065,8 @@ trimthenstep6:
else if (++tp->t_dupacks == TCPREXMTTHRESH) {
tcp_seq onxt = tp->snd_nxt;
u_int win =
-   min(tp->snd_wnd, tp->snd_cwnd) / 2 /
-   tp->t_maxseg;
+MIN(tp->snd_wnd, tp->snd_cwnd) 
/
+2 / tp-&

[Qemu-devel] [PULL 2/2] slirp: support dynamic block size for TFTP transfers

[Samuel Thibault]

From: Hervé Poussineau <hpous...@reactos.org>

The blocksize option is defined in RFC 1783 and RFC 2348.
We now support block sizes between 1 and 1428 bytes, instead of 512 only.

Signed-off-by: Hervé Poussineau <hpous...@reactos.org>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/tftp.c | 26 ++
 slirp/tftp.h |  8 +---
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index c1859066cc..50e714807d 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -72,6 +72,7 @@ static int tftp_session_allocate(Slirp *slirp, struct 
sockaddr_storage *srcsas,
   memset(spt, 0, sizeof(*spt));
   spt->client_addr = *srcsas;
   spt->fd = -1;
+  spt->block_size = 512;
   spt->client_port = tp->udp.uh_sport;
   spt->slirp = slirp;
 
@@ -115,7 +116,7 @@ static int tftp_read_data(struct tftp_session *spt, 
uint32_t block_nr,
 }
 
 if (len) {
-lseek(spt->fd, block_nr * 512, SEEK_SET);
+lseek(spt->fd, block_nr * spt->block_size, SEEK_SET);
 
 bytes_read = read(spt->fd, buf, len);
 }
@@ -189,7 +190,8 @@ static int tftp_send_oack(struct tftp_session *spt,
   values[i]) + 1;
 }
 
-m->m_len = sizeof(struct tftp_t) - 514 + n - sizeof(struct udphdr);
+m->m_len = sizeof(struct tftp_t) - (TFTP_BLOCKSIZE_MAX + 2) + n
+   - sizeof(struct udphdr);
 tftp_udp_output(spt, m, recv_tp);
 
 return 0;
@@ -214,7 +216,7 @@ static void tftp_send_error(struct tftp_session *spt,
   tp->x.tp_error.tp_error_code = htons(errorcode);
   pstrcpy((char *)tp->x.tp_error.tp_msg, sizeof(tp->x.tp_error.tp_msg), msg);
 
-  m->m_len = sizeof(struct tftp_t) - 514 + 3 + strlen(msg)
+  m->m_len = sizeof(struct tftp_t) - (TFTP_BLOCKSIZE_MAX + 2) + 3 + strlen(msg)
  - sizeof(struct udphdr);
   tftp_udp_output(spt, m, recv_tp);
 
@@ -240,7 +242,8 @@ static void tftp_send_next_block(struct tftp_session *spt,
   tp->tp_op = htons(TFTP_DATA);
   tp->x.tp_data.tp_block_nr = htons((spt->block_nr + 1) & 0x);
 
-  nobytes = tftp_read_data(spt, spt->block_nr, tp->x.tp_data.tp_buf, 512);
+  nobytes = tftp_read_data(spt, spt->block_nr, tp->x.tp_data.tp_buf,
+   spt->block_size);
 
   if (nobytes < 0) {
 m_free(m);
@@ -252,10 +255,11 @@ static void tftp_send_next_block(struct tftp_session *spt,
 return;
   }
 
-  m->m_len = sizeof(struct tftp_t) - (512 - nobytes) - sizeof(struct udphdr);
+  m->m_len = sizeof(struct tftp_t) - (TFTP_BLOCKSIZE_MAX - nobytes)
+ - sizeof(struct udphdr);
   tftp_udp_output(spt, m, recv_tp);
 
-  if (nobytes == 512) {
+  if (nobytes == spt->block_size) {
 tftp_session_update(spt);
   }
   else {
@@ -385,13 +389,11 @@ static void tftp_handle_rrq(Slirp *slirp, struct 
sockaddr_storage *srcsas,
   } else if (strcasecmp(key, "blksize") == 0) {
   int blksize = atoi(value);
 
-  /* If blksize option is bigger than what we will
-   * emit, accept the option with our packet size.
-   * Otherwise, simply do as we didn't see the option.
-   */
-  if (blksize >= 512) {
+  /* Accept blksize up to our maximum size */
+  if (blksize > 0) {
+  spt->block_size = MIN(blksize, TFTP_BLOCKSIZE_MAX);
   option_name[nb_options] = "blksize";
-  option_value[nb_options] = 512;
+  option_value[nb_options] = spt->block_size;
   nb_options++;
   }
   }
diff --git a/slirp/tftp.h b/slirp/tftp.h
index 2cd276dec6..a4c4a64e64 100644
--- a/slirp/tftp.h
+++ b/slirp/tftp.h
@@ -15,6 +15,7 @@
 #define TFTP_OACK   6
 
 #define TFTP_FILENAME_MAX 512
+#define TFTP_BLOCKSIZE_MAX 1428
 
 struct tftp_t {
   struct udphdr udp;
@@ -22,13 +23,13 @@ struct tftp_t {
   union {
 struct {
   uint16_t tp_block_nr;
-  uint8_t tp_buf[512];
+  uint8_t tp_buf[TFTP_BLOCKSIZE_MAX];
 } tp_data;
 struct {
   uint16_t tp_error_code;
-  uint8_t tp_msg[512];
+  uint8_t tp_msg[TFTP_BLOCKSIZE_MAX];
 } tp_error;
-char tp_buf[512 + 2];
+char tp_buf[TFTP_BLOCKSIZE_MAX + 2];
   } x;
 } __attribute__((packed));
 
@@ -36,6 +37,7 @@ struct tftp_session {
 Slirp *slirp;
 char *filename;
 int fd;
+uint16_t block_size;
 
 struct sockaddr_storage client_addr;
 uint16_t client_port;
-- 
2.11.0

[Qemu-devel] [PULL 0/2] slirp updates: MIN/MAX, tftp dynamic blocks

[Samuel Thibault]

The following changes since commit 82ecffa8c050bf5bbc13329e9b65eac1caa5b55c:

  Open 2.9 development tree (2016-12-20 16:20:16 +)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to 9443598d7e2f2c0a6493d97b3f11dd04837b08e8:

  slirp: support dynamic block size for TFTP transfers (2016-12-21 00:02:15 
+0100)


slirp updates


Hervé Poussineau (1):
  slirp: support dynamic block size for TFTP transfers

Yuval Shaia (1):
  slirp, disas: Replace min/max with MIN/MAX macros

 disas/m68k.c   |  8 ++--
 slirp/dhcpv6.c |  2 +-
 slirp/ip6_icmp.c   |  2 +-
 slirp/slirp.c  |  2 +-
 slirp/slirp.h  |  5 -
 slirp/tcp_input.c  | 16 
 slirp/tcp_output.c |  6 +++---
 slirp/tcp_timer.c  |  2 +-
 slirp/tcpip.h  |  2 +-
 slirp/tftp.c   | 26 ++
 slirp/tftp.h   |  8 +---
 11 files changed, 37 insertions(+), 42 deletions(-)

Re: [Qemu-devel] [PATCH v2] slirp, disas: Replace min/max with MIN/MAX macros

[Samuel Thibault]

I have applied it to my tree, thanks!

Samuel

Re: [Qemu-devel] [PATCH v2 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Hello,

Dr. David Alan Gilbert, on Mon 28 Nov 2016 09:08:16 +, wrote:
> Hmm, I don't really know IPv6 but I'm thinking this code will become 
> something like
> the following (says he not knowing whether a scope-id or a flowinfo
> is something that needs migrating) (untested):
> 
> 
> static const VMStateDescription vmstate_slirp_socket_addr = {
> .name = "slirp-socket-addr",
> .version_id = 4,
> .fields = (VMStateField[]) {
> VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
> VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
> slirp_family_inet),
> VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
> slirp_family_inet),
> 
> VMSTATE_BUFFER_TEST(sin6.sin6_addr, union slirp_sockaddr,
> slirp_family_inet6),
> VMSTATE_UINT16_TEST(sin6.sin6_port, union slirp_sockaddr,
> slirp_family_inet6),
> VMSTATE_UINT32_TEST(sin6.sin6_flowinfo, union slirp_sockaddr,
> slirp_family_inet6),
> VMSTATE_UINT32_TEST(sin6.sin6_scope_id, union slirp_sockaddr,
> slirp_family_inet6),
> 
> VMSTATE_END_OF_LIST()
> }
> };

Ok, that looks good :)

> So to me that looks pretty clean, we need to add another slirp_family_inet6
> test function, but then we just add the extra fields for the IPv6 stuff.

Yes, now I see.

> Can you suggest an IPv6 command line for testing that ?

Well, it doesn't exist yet, that's the problem. And applying your patch
would have made it to exist even harder, so that's why I was afraid.

I would say that your patch should contain these IPv6 lines, but as
comments since they are untested, for the person who will at some point
want to implement the IPv6 case here.

Samuel

Re: [Qemu-devel] [PATCH] cutils: Define min and max marcos

[Samuel Thibault]

Yuval Shaia, on Mon 28 Nov 2016 00:18:26 +0200, wrote:
> On Sun, Nov 27, 2016 at 03:10:04PM +0100, Samuel Thibault wrote:
> > Yuval Shaia, on Fri 25 Nov 2016 12:31:26 +0200, wrote:
> > > -#ifndef _WIN32
> > > -#define min(x,y) ((x) < (y) ? (x) : (y))
> > > -#define max(x,y) ((x) > (y) ? (x) : (y))
> > > -#endif
> > 
> > This has protection against _WIN32, I guess that was on purpose.
> 
> I'm not following.
> Are you suggesting that this was there to prevent code from compiling when
> _WIN32 was define?

I mean that min and max are already defined on windows:

https://msdn.microsoft.com/en-us/library/windows/desktop/dd757290%28v=vs.85%29.aspx

> > Perhaps qemu should avoid risking a clash with OS-provided min/max
> > macros, by renaming these to qemu_min/max?
> 
> On MIN and MAX?

Yes.

> I have noticed some other approach which was taken in osdep.h with ifdef,
> for example:
> 193 #ifndef ROUND_UP
> 194 #define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
> 195 #endif

That could probably be enough for our use indeed.

Samuel

Re: [Qemu-devel] [PATCH v2 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Samuel Thibault, on Sun 27 Nov 2016 16:13:46 +0100, wrote:
> Dr. David Alan Gilbert (git), on Wed 23 Nov 2016 18:52:57 +, wrote:
> > +static const VMStateDescription vmstate_slirp_socket_addr = {
> > +.name = "slirp-socket-addr",
> > +.version_id = 4,
> > +.fields = (VMStateField[]) {
> > +VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
> > +VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
> > +slirp_family_inet),
> > +VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
> > +slirp_family_inet),
> > +VMSTATE_END_OF_LIST()
> > +}
> > +};
> 
> How will we be able to add the IPv6 case here?

Reading again your previous post, it seemed it'd be in
slirp_family_inet, but I don't immediately see how.

Applying your patch for 2.9 would thus make porting the code to IPv6
more difficult than how it is now, so I'm quite reluctant :)

Could you perhaps simply add the IPv6 case in your patch series already?
It shouldn't be much work for you who actually know how the VMSTATE
machinery is supposed to work (I guess the amount of people who care
about slirp *and* know about VMSTATE is extremely small), and a proof of
concept for the portability to non-ipv4 addresse spaces.

Samuel

Re: [Qemu-devel] [PATCH v2 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Hello,

Dr. David Alan Gilbert (git), on Wed 23 Nov 2016 18:52:57 +, wrote:
> +static const VMStateDescription vmstate_slirp_socket_addr = {
> +.name = "slirp-socket-addr",
> +.version_id = 4,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
> +VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
> +slirp_family_inet),
> +VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
> +slirp_family_inet),
> +VMSTATE_END_OF_LIST()
> +}
> +};

How will we be able to add the IPv6 case here?

Samuel

Re: [Qemu-devel] [PATCH] cutils: Define min and max marcos

[Samuel Thibault]

Hello,

Yuval Shaia, on Fri 25 Nov 2016 12:31:26 +0200, wrote:
> -#ifndef _WIN32
> -#define min(x,y) ((x) < (y) ? (x) : (y))
> -#define max(x,y) ((x) > (y) ? (x) : (y))
> -#endif

This has protection against _WIN32, I guess that was on purpose.
Perhaps qemu should avoid risking a clash with OS-provided min/max
macros, by renaming these to qemu_min/max?

Samuel

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Samuel Thibault]

Stefan Hajnoczi, on Mon 21 Nov 2016 14:46:16 +, wrote:
> On Sun, Nov 20, 2016 at 09:41:36AM +0100, Vincent Bernat wrote:
> > From: Vincent Bernat 
> > 
> > Some network equipments are requesting a file using the netascii
> > protocol and this is not configurable. Currently, qemu's tftpd only
> > supports the octet protocol. This commit makes it accept the netascii
> > protocol as well but do not perform the requested transformation (LF ->
> > CR,LF) as it would be far more complex. The current implementation is
> > good enough. A user has always the choice to preencode the served file
> > correctly.
> > 
> > Signed-off-by: Vincent Bernat 
> > ---
> >  slirp/tftp.c | 11 ---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/slirp/tftp.c b/slirp/tftp.c
> > index c1859066ccb2..6907d5b92074 100644
> > --- a/slirp/tftp.c
> > +++ b/slirp/tftp.c
> > @@ -26,6 +26,7 @@
> >  #include "slirp.h"
> >  #include "qemu-common.h"
> >  #include "qemu/cutils.h"
> > +#include "qemu/log.h"
> >  
> >  static inline int tftp_session_in_use(struct tftp_session *spt)
> >  {
> > @@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct 
> > sockaddr_storage *srcsas,
> >  return;
> >}
> >  
> > -  if (strcasecmp(>x.tp_buf[k], "octet") != 0) {
> > +  if (strcasecmp(>x.tp_buf[k], "octet") == 0) {
> > +  k += 6;
> > +  } else if (strcasecmp(>x.tp_buf[k], "netascii") == 0) {
> > +  qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
> > +"no CR-LF conversion\n");
> > +  k += 9;
> > +  } else {
> 
> This is an RFC violation.  I don't think it's suitable for upstream QEMU.
> 
> The commit description says it would be "far more complex" to implement
> netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so hard?

I guess the question is that while the patch above could be accepted for
the upcoming 2.8 (I don't see it breaking existing systems), a patch
that would implement the transformation would be a lot more involved,
and really not suitable for 2.8.

Samuel

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

From: Vincent Bernat <vinc...@bernat.im>

Some network equipments are requesting a file using the netascii
protocol and this is not configurable. Currently, qemu's tftpd only
supports the octet protocol. This commit makes it accept the netascii
protocol as well but do not perform the requested transformation (LF ->
CR,LF) as it would be far more complex. The current implementation is
good enough. A user has always the choice to preencode the served file
correctly.

Signed-off-by: Vincent Bernat <vinc...@bernat.im>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/tftp.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index c185906..6907d5b 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -26,6 +26,7 @@
 #include "slirp.h"
 #include "qemu-common.h"
 #include "qemu/cutils.h"
+#include "qemu/log.h"
 
 static inline int tftp_session_in_use(struct tftp_session *spt)
 {
@@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct 
sockaddr_storage *srcsas,
 return;
   }
 
-  if (strcasecmp(>x.tp_buf[k], "octet") != 0) {
+  if (strcasecmp(>x.tp_buf[k], "octet") == 0) {
+  k += 6;
+  } else if (strcasecmp(>x.tp_buf[k], "netascii") == 0) {
+  qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
+"no CR-LF conversion\n");
+  k += 9;
+  } else {
   tftp_send_error(spt, 4, "Unsupported transfer mode", tp);
   return;
   }
 
-  k += 6; /* skipping octet */
-
   /* do sanity checks on the filename */
   if (!strncmp(req_fname, "../", 3) ||
   req_fname[strlen(req_fname) - 1] == '/' ||
-- 
2.10.2

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

The following changes since commit b0bcc86d2a87456f5a276f941dc775b265b309cf:

  Update version for v2.8.0-rc0 release (2016-11-15 20:55:12 +)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to b4a952793033e322f9ab0a3661b3faeee17ba4e6:

  tftp: fake support for netascii protocol (2016-11-20 18:02:38 +0100)


slirp updates


Vincent Bernat (1):
  tftp: fake support for netascii protocol

 slirp/tftp.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

Re: [Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

Vincent Bernat, on Sat 19 Nov 2016 09:03:08 +0100, wrote:
>  ❦ 19 novembre 2016 08:32 +0100, Thomas Huth  :
> 
> >> Some network equipments are requesting a file using the netascii
> >> protocol and this is not configurable. Currently, qemu's tftpd only
> >> supports the octet protocol. This commit makes it accept the netascii
> >> protocol as well but do not perform the requested transformation (LF ->
> >> CR,LF) as it would be far more complex.
> >
> > That sounds somewhat wrong to me. QEMU now seems to support a transfer
> > mode that is not really implemented.
> 
> On the other hand, the current implementation may not RFC compliant as
> the three modes (netascii, octet, mail) are not supported (but the RFC
> predates the usage of MAY/SHOULD/MUST, so it's unclear for me).
> 
> I have tried to add proper support, but this is more invasive as we
> cannot just seek in the file to get each block. However, this is
> something that I can do if compliance is important for QEMU.
> 
> > I think you should at least issue a qemu_log_mask(LOG_UNIMP, "...")
> > call in that case.
> 
> I can do that if needed.

That'd be better indeed. Otherwise people might wonder why things are
not working. Warning that they have to do the LF -> CR,LF conversion by
hand is important here.

Samuel

Re: [Qemu-devel] [PATCH for-2.8] curses: Fix compiler warnings (Mingw-w64 redefinition of macro KEY_EVENT)

[Samuel Thibault]

Stefan Weil, on Sat 19 Nov 2016 19:53:18 +0100, wrote:
> For builds with Mingw-w64 as it is included in Cygwin, there are two
> header files which define KEY_EVENT with different values.
> 
> This results in lots of compiler warnings like this one:
> 
>   CC  vl.o
> In file included from /qemu/include/ui/console.h:340:0,
>  from /qemu/vl.c:76:
> /usr/i686-w64-mingw32/sys-root/mingw/include/curses.h:1522:0: warning: 
> "KEY_EVENT" redefined
>  #define KEY_EVENT 0633  /* We were interrupted by an event */
> 
> In file included from /usr/share/mingw-w64/include/windows.h:74:0,
>  from /usr/share/mingw-w64/include/winsock2.h:23,
>  from /qemu/include/sysemu/os-win32.h:29,
>  from /qemu/include/qemu/osdep.h:100,
>  from /qemu/vl.c:24:
> /usr/share/mingw-w64/include/wincon.h:101:0: note: this is the location of 
> the previous definition
>  #define KEY_EVENT 0x1
> 
> QEMU only uses the KEY_EVENT macro from wincon.h.
> Therefore we can undefine the macro coming from curses.h.
> 
> The explicit include statement for curses.h in ui/curses.c is not needed
> and was removed.
> 
> Those two modifications fix the redefinition warnings.
> 
> Signed-off-by: Stefan Weil <s...@weilnetz.de>

Acked-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

> ---
>  include/ui/console.h | 3 +++
>  ui/curses.c  | 1 -
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index e2589e2..e5ae506 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -337,7 +337,10 @@ static inline pixman_format_code_t 
> surface_format(DisplaySurface *s)
>  }
>  
>  #ifdef CONFIG_CURSES
> +/* KEY_EVENT is defined in wincon.h and in curses.h. Avoid redefinition. */
> +#undef KEY_EVENT
>  #include 
> +#undef KEY_EVENT
>  typedef chtype console_ch_t;
>  extern chtype vga_to_curses[];
>  #else
> diff --git a/ui/curses.c b/ui/curses.c
> index 2e132a7..03cefdf 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -22,7 +22,6 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> -#include 
>  
>  #ifndef _WIN32
>  #include 
> -- 
> 2.10.2
> 
> 

-- 
Samuel
Now I know someone out there is going to claim, "Well then, UNIX is intuitive,
because you only need to learn 5000 commands, and then everything else follows
from that! Har har har!"
(Andy Bates in comp.os.linux.misc, on "intuitive interfaces", slightly
defending Macs.)

Re: [Qemu-devel] [PATCH] tftp: fake support for netascii protocol

[Samuel Thibault]

Hello,

Vincent Bernat, on Thu 17 Nov 2016 13:22:32 +0100, wrote:
>  ❦ 17 novembre 2016 13:20 +0100, Vincent Bernat  :
> 
> > Some network equipments are requesting a file using the netascii
> > protocol and this is not configurable. Currently, qemu's tftpd only
> > supports the octet protocol. This commit makes it accept the netascii
> > protocol as well but do not perform the requested transformation (LF ->
> > CR,LF) as it would be far more complex. The current implementation is
> > good enough. A user has always the choice to preencode the served file
> > correctly.
> 
> Signed-off-by: Vincent Bernat 

Thanks, I've pushed to my tree and requested a pull.

Samuel

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

From: Vincent Bernat <vinc...@bernat.im>

Some network equipments are requesting a file using the netascii
protocol and this is not configurable. Currently, qemu's tftpd only
supports the octet protocol. This commit makes it accept the netascii
protocol as well but do not perform the requested transformation (LF ->
CR,LF) as it would be far more complex. The current implementation is
good enough. A user has always the choice to preencode the served file
correctly.

Signed-off-by: Vincent Bernat <vinc...@bernat.im>
Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 slirp/tftp.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index c185906..ab1c05d 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -326,13 +326,15 @@ static void tftp_handle_rrq(Slirp *slirp, struct 
sockaddr_storage *srcsas,
 return;
   }
 
-  if (strcasecmp(>x.tp_buf[k], "octet") != 0) {
+  if (strcasecmp(>x.tp_buf[k], "octet") == 0) {
+  k += 6;
+  } else if (strcasecmp(>x.tp_buf[k], "netascii") == 0) {
+  k += 9;
+  } else {
   tftp_send_error(spt, 4, "Unsupported transfer mode", tp);
   return;
   }
 
-  k += 6; /* skipping octet */
-
   /* do sanity checks on the filename */
   if (!strncmp(req_fname, "../", 3) ||
   req_fname[strlen(req_fname) - 1] == '/' ||
-- 
2.10.2

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

The following changes since commit b0bcc86d2a87456f5a276f941dc775b265b309cf:

  Update version for v2.8.0-rc0 release (2016-11-15 20:55:12 +)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to 43fccf87c92a6a88a7294597b719f17fd1b41d3d:

  tftp: fake support for netascii protocol (2016-11-18 18:49:02 +0100)


slirp updates


Vincent Bernat (1):
  tftp: fake support for netascii protocol

 slirp/tftp.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

[Qemu-devel] [PULL] slirp: Fix access to freed memory

[Samuel Thibault]

The following changes since commit 83c83f9a5266ff113060f887f106a47920fa6974:

  Merge remote-tracking branch 'bonzini/tags/for-upstream' into staging 
(2016-11-11 12:51:50 +)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to ea64d5f08817b5e79e17135dce516c7583107f91:

  slirp: Fix access to freed memory (2016-11-14 17:36:33 +0100)


slirp updates


Samuel Thibault (1):
  slirp: Fix access to freed memory

 slirp/socket.c | 17 +
 1 file changed, 17 insertions(+)

[Qemu-devel] [PULL] slirp: Fix access to freed memory

[Samuel Thibault]

if_start() goes through the slirp->if_fastq and slirp->if_batchq
list of pending messages, and accesses ifm->ifq_so->so_nqueued of its
elements if ifm->ifq_so != NULL.  When freeing a socket, we thus need
to make sure that any pending message for this socket does not refer
to the socket any more.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Tested-by: Brian Candler <b.cand...@pobox.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
---
 slirp/socket.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/slirp/socket.c b/slirp/socket.c
index 280050a..6c18971 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -66,6 +66,23 @@ void
 sofree(struct socket *so)
 {
   Slirp *slirp = so->slirp;
+  struct mbuf *ifm;
+
+  for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
+   (struct quehead *) ifm != >if_fastq;
+   ifm = ifm->ifq_next) {
+if (ifm->ifq_so == so) {
+  ifm->ifq_so = NULL;
+}
+  }
+
+  for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
+   (struct quehead *) ifm != >if_batchq;
+   ifm = ifm->ifq_next) {
+if (ifm->ifq_so == so) {
+  ifm->ifq_so = NULL;
+}
+  }
 
   if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);
-- 
2.10.2

Re: [Qemu-devel] [PATCH] slirp: Fix access to freed memory

[Samuel Thibault]

Hello,

Note:

no-re...@patchew.org, on Sun 13 Nov 2016 15:13:47 -0800, wrote:
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> === OUTPUT BEGIN ===
> fatal: unrecognized argument: --no-patch
> Checking PATCH 1/1: ...
> ERROR: suspect code indent for conditional statements (4, 6)
> #29: FILE: slirp/socket.c:74:
> +if (ifm->ifq_so == so) {
> +  ifm->ifq_so = NULL;
> 
> ERROR: suspect code indent for conditional statements (4, 6)
> #37: FILE: slirp/socket.c:82:
> +if (ifm->ifq_so == so) {
> +  ifm->ifq_so = NULL;

This is due to that portion of the slirp code using 2-space indentation
instead of 4-space indentation.

Samuel

[Qemu-devel] [PATCH] slirp: Fix access to freed memory

[Samuel Thibault]

if_start() goes through the slirp->if_fastq and slirp->if_batchq
list of pending messages, and accesses ifm->ifq_so->so_nqueued of its
elements if ifm->ifq_so != NULL.  When freeing a socket, we thus need
to make sure that any pending message for this socket does not refer
to the socket any more.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Tested-by: Brian Candler <b.cand...@pobox.com>
---
 slirp/socket.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/slirp/socket.c b/slirp/socket.c
index 280050a..6c18971 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -66,6 +66,23 @@ void
 sofree(struct socket *so)
 {
   Slirp *slirp = so->slirp;
+  struct mbuf *ifm;
+
+  for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
+   (struct quehead *) ifm != >if_fastq;
+   ifm = ifm->ifq_next) {
+if (ifm->ifq_so == so) {
+  ifm->ifq_so = NULL;
+}
+  }
+
+  for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
+   (struct quehead *) ifm != >if_batchq;
+   ifm = ifm->ifq_next) {
+if (ifm->ifq_so == so) {
+  ifm->ifq_so = NULL;
+}
+  }
 
   if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);
-- 
2.10.2

Re: [Qemu-devel] Crashing in tcp_close

[Samuel Thibault]

Hello,

Brian Candler, on Sat 12 Nov 2016 09:33:55 +, wrote:
> On 11/11/2016 22:09, Samuel Thibault wrote:
> >Ooh, I see.  Now it's obvious, now that it's not coming from the tcb
> >loop:)  Could you try the attached patch?
> 
> It looks like it now goes into an infinite loop when a connection is closed.

Oops, sorry, my patch was completely bogus, here is a proper one.

Samuel
diff --git a/slirp/socket.c b/slirp/socket.c
index 280050a..6c18971 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -66,6 +66,23 @@ void
 sofree(struct socket *so)
 {
   Slirp *slirp = so->slirp;
+  struct mbuf *ifm;
+
+  for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
+   (struct quehead *) ifm != >if_fastq;
+   ifm = ifm->ifq_next) {
+if (ifm->ifq_so == so) {
+  ifm->ifq_so = NULL;
+}
+  }
+
+  for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
+   (struct quehead *) ifm != >if_batchq;
+   ifm = ifm->ifq_next) {
+if (ifm->ifq_so == so) {
+  ifm->ifq_so = NULL;
+}
+  }
 
   if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);

Re: [Qemu-devel] Crashing in tcp_close

[Samuel Thibault]

Brian Candler, on Fri 11 Nov 2016 20:53:12 +, wrote:
> On 11/11/2016 16:17, Samuel Thibault wrote:
> >Could you increase the value given to valgrind's --num-callers= so we
> >can make sure the context of this call?
> 
> OK: re-run with --num-callers=250. It took a few iterations, but I captured
> it again. (I have grepped out all the "invalid file descriptor" lines).

Thanks!

> ==1217== Thread 1:
> ==1217== Invalid read of size 4
> ==1217==at 0x550B5B: if_start (if.c:230)
> ==1217==by 0x5550E2: slirp_pollfds_poll (slirp.c:770)
> ==1217==by 0x5891EB: main_loop_wait (main-loop.c:508)
> ==1217==by 0x2F4430: main_loop (vl.c:1908)
> ==1217==by 0x2F4430: main (vl.c:4604)

Ooh, I see.  Now it's obvious, now that it's not coming from the tcb
loop :) Could you try the attached patch?

Samuel
diff --git a/slirp/socket.c b/slirp/socket.c
index 280050a..1a50d30 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -66,6 +66,13 @@ void
 sofree(struct socket *so)
 {
   Slirp *slirp = so->slirp;
+  struct mbuf *ifm;
+
+  for (ifm = slirp->next_m; ifm; ifm = ifm->ifq_next) {
+if (ifm->ifq_so == so) {
+  ifm->ifq_so = NULL;
+}
+  }
 
   if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);

Re: [Qemu-devel] Crashing in tcp_close

[Samuel Thibault]

Hello,

Brian Candler, on Fri 11 Nov 2016 16:02:44 +, wrote:
> Aha!! Looking carefully at valgrind output, I see some definite cases of
> use-after-free in tcp_output. Does the info below help?

Ok, that's interesting. I however still don't see how that could happen
:)

> ==18350== Invalid read of size 4
> ==18350==at 0x550B5B: if_start (if.c:230)
> ==18350==by 0x552E6C: ip_output (ip_output.c:85)
> ==18350==by 0x55AA31: tcp_output (tcp_output.c:469)
> ==18350==by 0x558FD7: tcp_input (tcp_input.c:1386)
> ==18350==by 0x55543F: slirp_input (slirp.c:867)
> ==18350==by 0x54AFBF: net_slirp_receive (slirp.c:118)
> ==18350==by 0x540B18: nc_sendv_compat (net.c:701)
> ==18350==by 0x540B18: qemu_deliver_packet_iov (net.c:728)
> ==18350==by 0x5438DA: qemu_net_queue_deliver_iov (queue.c:179)
> ==18350==by 0x5438DA: qemu_net_queue_send_iov (queue.c:224)
> ==18350==by 0x36B428: virtio_net_flush_tx (virtio-net.c:1282)
> ==18350==by 0x36B624: virtio_net_tx_bh (virtio-net.c:1387)
> ==18350==by 0x5804EC: aio_bh_call (async.c:67)
> ==18350==by 0x5804EC: aio_bh_poll (async.c:95)
> ==18350==by 0x58A8FF: aio_dispatch (aio-posix.c:308)

Could you increase the value given to valgrind's --num-callers= so we
can make sure the context of this call?  Here tcp_input get the buffer
being freed below from the slirp->tcb list, and sofree happens to drop
it from that list before calling free...

I'm wondering whether we have a kind of concurrency or recursivity here.

> ==18350==  Address 0x9eabec4 is 340 bytes inside a block of size 432 free'd
> ==18350==at 0x4C2EDEB: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18350==by 0x55B25E: tcp_close (tcp_subr.c:334)
> ==18350==by 0x55C7AE: tcp_timers (tcp_timer.c:289)
> ==18350==by 0x55C7AE: tcp_slowtimo (tcp_timer.c:89)
> ==18350==by 0x555187: slirp_pollfds_poll (slirp.c:576)
> ==18350==by 0x5891EB: main_loop_wait (main-loop.c:508)
> ==18350==by 0x2F4430: main_loop (vl.c:1908)
> ==18350==by 0x2F4430: main (vl.c:4604)

Samuel

[Qemu-devel] [PATCH] Fix cursesw detection

[Samuel Thibault]

On systems which do not provide ncursesw.pc and whose /usr/include/curses.h
does not include wide support, we should not only try with no -I, i.e.
/usr/include, but also with -I/usr/include/ncursesw.

To properly detect for wide support with and without -Werror, we need to
check for the presence of e.g. the WACS_DEGREE macro.

We also want to stop at the first curses_inc_list configuration which works,
and make sure to set IFS to : at each new loop.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Tested-by: Cornelia Huck <cornelia.h...@de.ibm.com>
---
 configure | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index fd6f898..7d2a34e 100755
--- a/configure
+++ b/configure
@@ -2926,7 +2926,7 @@ if test "$curses" != "no" ; then
 curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
 curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
   else
-curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
+curses_inc_list="$($pkg_config --cflags ncursesw 
2>/dev/null):-I/usr/include/ncursesw:"
 curses_lib_list="$($pkg_config --libs ncursesw 
2>/dev/null):-lncursesw:-lcursesw"
   fi
   curses_found=no
@@ -2941,11 +2941,13 @@ int main(void) {
   resize_term(0, 0);
   addwstr(L"wide chars\n");
   addnwstr(, 1);
+  add_wch(WACS_DEGREE);
   return s != 0;
 }
 EOF
   IFS=:
   for curses_inc in $curses_inc_list; do
+IFS=:
 for curses_lib in $curses_lib_list; do
   unset IFS
   if compile_prog "$curses_inc" "$curses_lib" ; then
@@ -2955,6 +2957,9 @@ EOF
 break
   fi
 done
+if test "$curses_found" = yes ; then
+  break
+fi
   done
   unset IFS
   if test "$curses_found" = "yes" ; then
-- 
2.10.2

Re: [Qemu-devel] [PATCH] Fix legacy ncurses detection.

[Samuel Thibault]

Sergey Smolov, on Wed 09 Nov 2016 13:15:18 +0300, wrote:
> Is it planned to publish this patch into master?

Yes.

Samuel

Re: [Qemu-devel] [PATCH] Fix legacy ncurses detection.

[Samuel Thibault]

Hello,

Cornelia Huck, on Wed 09 Nov 2016 10:40:28 +0100, wrote:
> Still curses=no... log attached.

Oops, sorry, I misplaced my code, and it somehow worked in my case.
Could you give a try at the attached patch instead?

Thanks,
Samuel
commit cc8965eb848f53599895a650a6e062319189be1f
Author: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Date:   Tue Nov 8 20:57:27 2016 +0100

Fix cursesw detection

On systems which do not provide ncursesw.pc and whose /usr/include/curses.h
does not include wide support, we should not only try with no -I, i.e.
/usr/include, but also with -I/usr/include/ncursesw.

To properly detect for wide support with and without -Werror, we need to
check for the presence of e.g. the WACS_DEGREE macro.

We also want to stop at the first curses_inc_list configuration which works,
and make sure to set IFS to : at each new loop.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

diff --git a/configure b/configure
index fd6f898..7d2a34e 100755
--- a/configure
+++ b/configure
@@ -2926,7 +2926,7 @@ if test "$curses" != "no" ; then
 curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
 curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
   else
-curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
+curses_inc_list="$($pkg_config --cflags ncursesw 
2>/dev/null):-I/usr/include/ncursesw:"
 curses_lib_list="$($pkg_config --libs ncursesw 
2>/dev/null):-lncursesw:-lcursesw"
   fi
   curses_found=no
@@ -2941,11 +2941,13 @@ int main(void) {
   resize_term(0, 0);
   addwstr(L"wide chars\n");
   addnwstr(, 1);
+  add_wch(WACS_DEGREE);
   return s != 0;
 }
 EOF
   IFS=:
   for curses_inc in $curses_inc_list; do
+IFS=:
 for curses_lib in $curses_lib_list; do
   unset IFS
   if compile_prog "$curses_inc" "$curses_lib" ; then
@@ -2955,6 +2957,9 @@ EOF
 break
   fi
 done
+if test "$curses_found" = yes ; then
+  break
+fi
   done
   unset IFS
   if test "$curses_found" = "yes" ; then

Re: [Qemu-devel] [PATCH] Fix legacy ncurses detection.

[Samuel Thibault]

Hello,

Cornelia Huck, on Wed 09 Nov 2016 10:12:36 +0100, wrote:
> On Wed, 9 Nov 2016 10:04:02 +0100
> Samuel Thibault <samuel.thiba...@gnu.org> wrote:
> > Please post config.log so we can have a clue about what is going
> > wrong.  All these error messages are meant to be reported verbatim, not
> > reinterpreted :)
> 
> Well, no error here - just curses=no.

The errors are in config.log, that's what one is supposed to look at
when there are configure issues.

> config.log attached. The difference seems to be that the statement you
> added in the sample program causes a real error instead of a warning.

Yes, that was on purpose, to avoid the -Werror issue. It's expected to
happen because /usr/include/curses.h doesn't seem to have the wide
support. But -I/usr/include/ncursesw/curses.h is supposed to have. But:

> cc -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels 
> -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
> -Wold-style-definition -Wtype-limits -fstack-protector-all 
> -I/usr/include/libpng16 -I/usr/include/ncursesw -o config-temp/qemu-conf.exe 
> config-temp/qemu-conf.c -m64 -g :-lncursesw:-lcursesw
> cc: error: :-lncursesw:-lcursesw: No such file or directory

That's what the real issue is in your case.  I now see why, could you
try the attached patch instead?

Samuel
commit ea32127ca780b0945827776bf27f99383529621c
Author: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Date:   Tue Nov 8 20:57:27 2016 +0100

Fix cursesw detection

On systems which do not provide ncursesw.pc and whose /usr/include/curses.h
does not include wide support, we should not only try with no -I, i.e.
/usr/include, but also with -I/usr/include/ncursesw.

To properly detect for wide support with and without -Werror, we need to
check for the presence of e.g. the WACS_DEGREE macro.

We also want to stop at the first curses_inc_list configuration which works,
    and make sure to set IFS to : at each new loop.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

diff --git a/configure b/configure
index fd6f898..bac7bcc 100755
--- a/configure
+++ b/configure
@@ -2926,7 +2926,7 @@ if test "$curses" != "no" ; then
 curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
 curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
   else
-curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
+curses_inc_list="$($pkg_config --cflags ncursesw 
2>/dev/null):-I/usr/include/ncursesw:"
 curses_lib_list="$($pkg_config --libs ncursesw 
2>/dev/null):-lncursesw:-lcursesw"
   fi
   curses_found=no
@@ -2941,6 +2941,7 @@ int main(void) {
   resize_term(0, 0);
   addwstr(L"wide chars\n");
   addnwstr(, 1);
+  add_wch(WACS_DEGREE);
   return s != 0;
 }
 EOF
@@ -2954,7 +2955,11 @@ EOF
 libs_softmmu="$curses_lib $libs_softmmu"
 break
   fi
+  IFS=:
 done
+if test "$curses_found" = yes ; then
+  break
+fi
   done
   unset IFS
   if test "$curses_found" = "yes" ; then

Re: [Qemu-devel] [PATCH] Fix legacy ncurses detection.

[Samuel Thibault]

Hello,

Cornelia Huck, on Wed 09 Nov 2016 09:58:59 +0100, wrote:
> On Tue, 8 Nov 2016 21:10:19 +0100
> Samuel Thibault <samuel.thiba...@gnu.org> wrote:
> > Cornelia Huck, on Tue 08 Nov 2016 12:34:49 +0100, wrote:
> > “
> > configure test passed without -Werror but failed with -Werror.
> > This is probably a bug in the configure script. The failing command
> > will be at the bottom of config.log.
> > You can run configure with --disable-werror to bypass this check.
> > ”
> > 
> > If so, you should really have said it, I was really wondering how
> > configure could just stopping in your case.  That does explain things
> > indeed.
> 
> I said so in my very first mail for the issue... appears I was unclear.

Do you mean "configure barfs about -Werror."?
Yes it was unclear to me :)

> > Could you try the attached patch?  It should be able to really fail
> > without Werror too.
> 
> With your patch, configure runs through and detects curses=no. Not sure
> that's correct, though: SLES12SP1 _does_ have curses, but not a .pc
> file for ncursesw. I don't know enough about curses to say whether it
> should be that way...

Please post config.log so we can have a clue about what is going
wrong.  All these error messages are meant to be reported verbatim, not
reinterpreted :)

Samuel

Re: [Qemu-devel] [PATCH] Fix legacy ncurses detection.

[Samuel Thibault]

Cornelia Huck, on Tue 08 Nov 2016 12:34:49 +0100, wrote:
> > diff --git a/configure b/configure
> > index fd6f898..e200aa8 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2926,7 +2926,7 @@ if test "$curses" != "no" ; then
> >  curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
> >  curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
> >else
> > -curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
> > +curses_inc_list="$($pkg_config --cflags ncursesw 
> > 2>/dev/null):-I/usr/include/ncursesw:"
> 
> This arrives at
> 
> curses_inc_list=":-I/usr/include/ncursesw:"
> 
> which causes the parser below to start with an empty curses_inc (with :
> as separator).

Yes, this is expected.

> configure fails as before (with -Werror; passes without).

Ah!
So are you getting the following message?

“
configure test passed without -Werror but failed with -Werror.
This is probably a bug in the configure script. The failing command
will be at the bottom of config.log.
You can run configure with --disable-werror to bypass this check.
”

If so, you should really have said it, I was really wondering how
configure could just stopping in your case.  That does explain things
indeed.

Could you try the attached patch?  It should be able to really fail
without Werror too.

> > @@ -2955,6 +2955,9 @@ EOF
> >  break
> >fi
> >  done
> > +if test "$curses_found" = yes ; then
> > +  break
> > +fi
> 
> Breaking out as soon as we've found a working config seems like a good
> idea, but I don't think it's related to this issue.

Actually it is: in your case it's the second config which will work, the
third one will fail. Not breaking would mean we keep the failing one.

Samuel
commit 4c5e78e8843fa919f2601d8e44029ed0e711c6d9
Author: Samuel Thibault <samuel.thiba...@ens-lyon.org>
Date:   Tue Nov 8 20:57:27 2016 +0100

Fix cursesw detection

On systems which do not provide ncursesw.pc and whose /usr/include/curses.h
does not include wide support, we should not only try with no -I, i.e.
/usr/include, but also with -I/usr/include/ncursesw.

To properly detect for wide support with and without -Werror, we need to
check for the presence of e.g. the WACS_DEGREE macro.

We also want to stop at the first curses_inc_list configuration which works.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

diff --git a/configure b/configure
index fd6f898..f35edf8 100755
--- a/configure
+++ b/configure
@@ -2926,7 +2926,7 @@ if test "$curses" != "no" ; then
 curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
 curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
   else
-curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
+curses_inc_list="$($pkg_config --cflags ncursesw 
2>/dev/null):-I/usr/include/ncursesw:"
 curses_lib_list="$($pkg_config --libs ncursesw 
2>/dev/null):-lncursesw:-lcursesw"
   fi
   curses_found=no
@@ -2941,6 +2941,7 @@ int main(void) {
   resize_term(0, 0);
   addwstr(L"wide chars\n");
   addnwstr(, 1);
+  add_wch(WACS_DEGREE);
   return s != 0;
 }
 EOF
@@ -2955,6 +2956,9 @@ EOF
 break
   fi
 done
+if test "$curses_found" = yes ; then
+  break
+fi
   done
   unset IFS
   if test "$curses_found" = "yes" ; then

Re: [Qemu-devel] [PATCH] Fix legacy ncurses detection.

[Samuel Thibault]

Hello,

On 7 November 2016 at 13:38, Michal Suchanek  wrote:
> -curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
> +curses_inc_list="$($pkg_config --cflags ncursesw 
> 2>/dev/null)-I/usr/include/ncursesw:"

Cornelia Huck, on Mon 07 Nov 2016 17:26:56 +0100, wrote:
> +if [ "$curses_inc_list" == ":" ]; then
> +   # some systems don't provide a proper .pc file for ncursesw
> +   curses_inc_list="-I/usr/include/ncursesw:"
> +fi

Could you rather try the attached patch?

Samuel
diff --git a/configure b/configure
index fd6f898..e200aa8 100755
--- a/configure
+++ b/configure
@@ -2926,7 +2926,7 @@ if test "$curses" != "no" ; then
 curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
 curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
   else
-curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
+curses_inc_list="$($pkg_config --cflags ncursesw 
2>/dev/null):-I/usr/include/ncursesw:"
 curses_lib_list="$($pkg_config --libs ncursesw 
2>/dev/null):-lncursesw:-lcursesw"
   fi
   curses_found=no
@@ -2955,6 +2955,9 @@ EOF
 break
   fi
 done
+if test "$curses_found" = yes ; then
+  break
+fi
   done
   unset IFS
   if test "$curses_found" = "yes" ; then

Re: [Qemu-devel] Crashing in tcp_close

[Samuel Thibault]

Hello,

Stefan Hajnoczi, on Fri 04 Nov 2016 11:14:19 +, wrote:
> CCing slirp maintainers to get attention on this bug

Thanks!

> > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> > 0x76a1bb5b in _int_free (av=0x76d5fb20 ,
> > p=, have_lock=0) at malloc.c:4006
> > 4006malloc.c: No such file or directory.
> > (gdb) bt
> > #0  0x76a1bb5b in _int_free (av=0x76d5fb20 ,
> > p=, have_lock=0)
> > at malloc.c:4006
> > #1  0x76a1fabc in __GI___libc_free (mem=) at
> > malloc.c:2969
> > #2  0x559a6c0f in tcp_close (tp=tp@entry=0x56621ed0) at
> > slirp/tcp_subr.c:334
> > #3  0x559a6c8f in tcp_drop (tp=tp@entry=0x56621ed0,
> > err=) at slirp/tcp_subr.c:298
> > #4  0x559a816b in tcp_timers (timer=,
> > tp=0x56621ed0) at slirp/tcp_timer.c:179
> > #3  0x559a6c8f in tcp_drop (tp=tp@entry=0x56621ed0,
> > err=) at slirp/tcp_subr.c:298
> > #4  0x559a816b in tcp_timers (timer=,
> > tp=0x56621ed0) at slirp/tcp_timer.c:179
> > #5  tcp_slowtimo (slirp=slirp@entry=0x5658ecf0) at slirp/tcp_timer.c:89

> > * If so, what additional gdb output would you like me to provide?
> 
> I wonder if this connection has already been closed/freed before and the
> timer fires shortly afterward.  That's just a guess based on the
> backtrace.

That's very unlikely: soclose removes the socket from the list, so
tcp_slowtimo wouldn't be able to find it. That'd rather be a buffer
overflow. But it's hard to believe it could come from the socket
structure since it doesn't contain any buffer.

Brian, could you run it with

export MALLOC_CHECK_=2

and also this could be useful:

export MALLOC_PERTURB_=1234

Also, to rule out the double-free scenario, and try to catch a buffer
overflow coming from the socket structure itself, I have attached a
patch which adds some debugging.

> > * If developers want to reproduce this, let me know and I can probably send
> > the VM qcow2 file and/or packer source privately off-list [I need to check
> > permission for that]

That could be useful.

Samuel
diff --git a/slirp/socket.c b/slirp/socket.c
index 280050a..e603164 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -51,10 +51,12 @@ socreate(Slirp *slirp)
   so = (struct socket *)malloc(sizeof(struct socket));
   if(so) {
 memset(so, 0, sizeof(struct socket));
+so->canary1 = 0xdeadbeef;
 so->so_state = SS_NOFDREF;
 so->s = -1;
 so->slirp = slirp;
 so->pollfds_idx = -1;
+so->canary2 = 0xbe3fd3ad;
   }
   return(so);
 }
@@ -67,6 +69,14 @@ sofree(struct socket *so)
 {
   Slirp *slirp = so->slirp;
 
+  if (so->s == -1234)
+fprintf(stderr,"oops, re-freeing a freed socket!\n");
+  if (so->canary1 != 0xdeadbeef)
+fprintf(stderr,"oops, canary1 bogus!\n");
+  if (so->canary2 != 0xbe3fd3ad)
+fprintf(stderr,"oops, canary2 bogus!\n");
+  so->s = -1234;
+
   if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);
so->extra=NULL;
diff --git a/slirp/socket.h b/slirp/socket.h
index 8feed2a..14fac1c 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -17,6 +17,7 @@
 
 struct socket {
   struct socket *so_next,*so_prev;  /* For a linked list of sockets */
+  int canary1;
 
   int s;   /* The actual socket */
 
@@ -70,6 +71,7 @@ struct socket {
   struct sbuf so_rcv;  /* Receive buffer */
   struct sbuf so_snd;  /* Send buffer */
   void * extra;/* Extra pointer */
+  int canary2;
 };
 
 

Re: [Qemu-devel] slirp: fix ipv6 guest network access with windows host

[Samuel Thibault]

Hello,

Bo Hu, on Wed 02 Nov 2016 16:02:29 -0700, wrote:
>     This patch is from android emulator (which is based on qemu2.2) and I hope
> this patch is
> also useful to upstream qemu as well.

Indeed, even if normally we fill all required fields, it's probably
better to memset the whole structure.

> From 021eac8c593a34a6a5e106d187a8e1fd22a1522f Mon Sep 17 00:00:00 2001
> From: bohu <[1]b...@google.com>
> Date: Wed, 2 Nov 2016 15:56:26 -0700
> Subject: [PATCH] slirp: fix ipv6 guest network access with windows host
> 
> In tcp_input function, local sockaddr_storage variables lhost
> and fhost are used without being cleared to zero; and consequently
> tcp connect call fails on windows because there is some random data
> in those variables (windows complains with WSAEADDRNOTAVAIL);
> 
> This CL calls memset to clear those two variables so that the address
> passed to connect does not have random data in it.
> 
> Signed-off-by: Bo Hu <[2]b...@google.com>

> +    memset(, 0, sizeof(lhost);
> +    memset(, 0, sizeof(fhost);

There is just a typo: missing closing parenthesis...

But apart from that,
Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

Samuel

Re: [Qemu-devel] [PATCH 4/4] gtk: set window ID

[Samuel Thibault]

Samuel Thibault, on Mon 31 Oct 2016 17:00:07 +0100, wrote:
> This uses the console API to record the window ID of the GTK windows.

Ah, sorry, I let this one through, please ignore it, it'll need rework
as discussed in the other thread.

Samuel

[Qemu-devel] [PATCH 2/4] console: move window ID code from baum to sdl

[Samuel Thibault]

This moves the SDL bits for window ID from the baum driver to SDL, as
well as fixing the build for non-X11.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 backends/baum.c | 25 +++--
 ui/sdl.c| 25 +
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/backends/baum.c b/backends/baum.c
index b92369d..5d7d27c 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -27,12 +27,10 @@
 #include "sysemu/char.h"
 #include "qemu/timer.h"
 #include "hw/usb.h"
+#include "ui/console.h"
 #include 
 #include 
 #include 
-#ifdef CONFIG_SDL
-#include 
-#endif
 
 #if 0
 #define DPRINTF(fmt, ...) \
@@ -227,11 +225,6 @@ static const uint8_t nabcc_translation[2][256] = {
 /* The guest OS has started discussing with us, finish initializing BrlAPI */
 static int baum_deferred_init(BaumDriverState *baum)
 {
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-SDL_SysWMinfo info;
-#endif
-#endif
 int tty;
 
 if (baum->deferred_init) {
@@ -243,21 +236,9 @@ static int baum_deferred_init(BaumDriverState *baum)
 return 0;
 }
 
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-memset(, 0, sizeof(info));
-SDL_VERSION();
-if (SDL_GetWMInfo()) {
-tty = info.info.x11.wmwindow;
-} else {
-#endif
-#endif
+tty = qemu_graphic_console_get_window_id();
+if (tty == -1)
 tty = BRLAPI_TTY_DEFAULT;
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-}
-#endif
-#endif
 
 if (brlapi__enterTtyMode(baum->brlapi, tty, NULL) == -1) {
 brlapi_perror("baum: brlapi__enterTtyMode");
diff --git a/ui/sdl.c b/ui/sdl.c
index d8cf5bc..8d9d171 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -947,6 +947,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 int flags;
 uint8_t data = 0;
 const SDL_VideoInfo *vi;
+SDL_SysWMinfo info;
 char *filename;
 
 #if defined(__APPLE__)
@@ -1023,5 +1024,29 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 sdl_cursor_hidden = SDL_CreateCursor(, , 8, 1, 0, 0);
 sdl_cursor_normal = SDL_GetCursor();
 
+memset(, 0, sizeof(info));
+SDL_VERSION();
+if (SDL_GetWMInfo()) {
+int i;
+for (i = 0; ; i++) {
+/* All consoles share the same window */
+QemuConsole *con = qemu_console_lookup_by_index(i);
+if (con) {
+#if defined(SDL_VIDEO_DRIVER_X11)
+qemu_console_set_window_id(i, info.info.x11.wmwindow);
+#elif defined(SDL_VIDEO_DRIVER_NANOX) || \
+  defined(SDL_VIDEO_DRIVER_WINDIB) || defined(SDL_VIDEO_DRIVER_DDRAW) || \
+  defined(SDL_VIDEO_DRIVER_GAPI) || \
+  defined(SDL_VIDEO_DRIVER_RISCOS)
+qemu_console_set_window_id(i, (int) (uintptr_t) info.window);
+#else
+qemu_console_set_window_id(i, info.data);
+#endif
+} else {
+break;
+}
+}
+}
+
 atexit(sdl_cleanup);
 }
-- 
2.10.1

[Qemu-devel] [PATCHv3 0/4] Move getting XWindow ID from baum driver to graphical backend

[Samuel Thibault]

Hello,

This is a respin of moving getting XWindow ID from baum to actual
graphical backends.  This only contains the new API, the move
of existing code to the new API, and the addition of support for
SDL2. Gtk will need more discussion and work.

Compared to v2, this fixes build on 64bit windows, where the window
handle needs to be explictly truncated to 32bit (such handles are
guaranteed to be 32bit)

Samuel

Samuel Thibault (4):
  console: add API to get underlying gui window ID
  console: move window ID code from baum to sdl
  sdl2: set window ID
  gtk: set window ID

 backends/baum.c  | 25 +++--
 include/ui/console.h |  3 +++
 ui/console.c | 15 +++
 ui/gtk.c | 25 +++--
 ui/sdl.c | 25 +
 ui/sdl2.c|  7 +++
 6 files changed, 76 insertions(+), 24 deletions(-)

-- 
2.10.1

[Qemu-devel] [PATCH 3/4] sdl2: set window ID

[Samuel Thibault]

This uses the console API to record the window ID of the SDL2 windows.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 ui/sdl2.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 30d2a3c..b464f16 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -761,6 +761,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 uint8_t data = 0;
 char *filename;
 int i;
+SDL_SysWMinfo info;
 
 if (no_frame) {
 gui_noframe = 1;
@@ -786,6 +787,8 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 exit(1);
 }
 SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
+memset(, 0, sizeof(info));
+SDL_VERSION();
 
 for (i = 0;; i++) {
 QemuConsole *con = qemu_console_lookup_by_index(i);
@@ -813,6 +816,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 #endif
 sdl2_console[i].dcl.con = con;
 register_displaychangelistener(_console[i].dcl);
+
+if (SDL_GetWindowWMInfo(sdl2_console[i].real_window, )) {
+qemu_console_set_window_id(i, info.info.x11.window);
+}
 }
 
 /* Load a 32x32x4 image. White pixels are transparent. */
-- 
2.10.1

[Qemu-devel] [PATCH 1/4] console: add API to get underlying gui window ID

[Samuel Thibault]

This adds two console functions, qemu_console_set_window_id and
qemu_graphic_console_get_window_id, to let graphical backend record the
window id in the QemuConsole structure, and let the baum driver read it.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 include/ui/console.h |  3 +++
 ui/console.c | 15 +++
 2 files changed, 18 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index e2589e2..cf07e41 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -394,6 +394,9 @@ uint32_t qemu_console_get_head(QemuConsole *con);
 QemuUIInfo *qemu_console_get_ui_info(QemuConsole *con);
 int qemu_console_get_width(QemuConsole *con, int fallback);
 int qemu_console_get_height(QemuConsole *con, int fallback);
+/* Return the low-level window id for the first graphical console */
+int qemu_graphic_console_get_window_id(void);
+void qemu_console_set_window_id(int index, int window_id);
 
 void console_select(unsigned int index);
 void qemu_console_resize(QemuConsole *con, int width, int height);
diff --git a/ui/console.c b/ui/console.c
index ed888e5..aa3c4c7 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -124,6 +124,7 @@ struct QemuConsole {
 int dcls;
 DisplayChangeListener *gl;
 bool gl_block;
+int window_id;
 
 /* Graphic console state.  */
 Object *device;
@@ -273,6 +274,20 @@ void graphic_hw_gl_block(QemuConsole *con, bool block)
 }
 }
 
+int qemu_graphic_console_get_window_id(void)
+{
+if (consoles[0]->console_type == GRAPHIC_CONSOLE) {
+return consoles[0]->window_id;
+}
+return -1;
+}
+
+void qemu_console_set_window_id(int index, int window_id)
+{
+assert(index >= 0 && index < nb_consoles);
+consoles[index]->window_id = window_id;
+}
+
 void graphic_hw_invalidate(QemuConsole *con)
 {
 if (!con) {
-- 
2.10.1

[Qemu-devel] [PATCH 4/4] gtk: set window ID

[Samuel Thibault]

This uses the console API to record the window ID of the GTK windows.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 ui/gtk.c | 25 +++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index ca737c4..d75f255 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2170,6 +2170,13 @@ void gtk_display_init(DisplayState *ds, bool 
full_screen, bool grab_on_hover)
 GtkDisplayState *s = g_malloc0(sizeof(*s));
 char *filename;
 GdkDisplay *window_display;
+GdkWindow *gdk_window;
+#ifdef GDK_WINDOWING_X11
+Window window_id;
+#elif defined(GDK_WINDOWING_WIN32)
+HWND window_id;
+#endif
+int i;
 
 if (!gtkinit) {
 fprintf(stderr, "gtk initialization failed\n");
@@ -2232,8 +2239,6 @@ void gtk_display_init(DisplayState *ds, bool full_screen, 
bool grab_on_hover)
 {
 VirtualConsole *cur = gd_vc_find_current(s);
 if (cur) {
-int i;
-
 for (i = 0; i < s->nb_vcs; i++) {
 VirtualConsole *vc = >vc[i];
 if (vc && vc->type == GD_VC_VTE && vc != cur) {
@@ -2253,6 +2258,22 @@ void gtk_display_init(DisplayState *ds, bool 
full_screen, bool grab_on_hover)
 }
 
 gd_set_keycode_type(s);
+
+gdk_window = gtk_widget_get_window(s->window);
+#ifdef GDK_WINDOWING_X11
+window_id = GDK_WINDOW_XID(gdk_window);
+#elif defined(GDK_WINDOWING_WIN32)
+window_id = gdk_win32_window_get_impl_hwnd(gdk_window);
+#endif
+for (i = 0; ; i++) {
+/* All consoles share the same window */
+QemuConsole *con = qemu_console_lookup_by_index(i);
+if (con) {
+qemu_console_set_window_id(i, (int) window_id);
+} else {
+break;
+}
+}
 }
 
 void early_gtk_display_init(int opengl)
-- 
2.10.1

Re: [Qemu-devel] [PULL 6/6] curses: Use cursesw instead of curses

[Samuel Thibault]

Cornelia Huck, on Mon 31 Oct 2016 14:08:48 +0100, wrote:
> On Mon, 31 Oct 2016 15:03:33 +0100
> Samuel Thibault <samuel.thiba...@gnu.org> wrote:
> 
> > Cornelia Huck, on Mon 31 Oct 2016 13:48:01 +0100, wrote:
> > > On Mon, 31 Oct 2016 13:39:30 +0100
> > > Samuel Thibault <samuel.thiba...@gnu.org> wrote:
> > > 
> > > > Cornelia Huck, on Mon 31 Oct 2016 13:08:06 +0100, wrote:
> > > > > You mean in configure, right? Including cursesw.h in the test program
> > > > > gets configure going again.
> > > > 
> > > > Could you try the attached patch which fixes both configure and
> > > > ui/curses.c?
> > > 
> > > Sadly, this does not fix it for me. I get the same errors in the
> > > configure test build as before.
> > 
> > Could you post the config.log so we get a better view at what is going
> > wrong?
> 
> Attached.

> cc -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels 
> -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
> -Wold-style-definition -Wtype-limits -fstack-protector-all 
> -I/usr/include/libpng16 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c 
> -m64 -g
> config-temp/qemu-conf.c: In function ‘main’:
> config-temp/qemu-conf.c:13:3: warning: implicit declaration of function 
> ‘addwstr’ [-Wimplicit-function-declaration]
...
> cc -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels 
> -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
> -Wold-style-definition -Wtype-limits -fstack-protector-all 
> -I/usr/include/libpng16 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c 
> -m64 -g -lncursesw
> config-temp/qemu-conf.c: In function ‘main’:
> config-temp/qemu-conf.c:13:3: warning: implicit declaration of function 
> ‘addwstr’ [-Wimplicit-function-declaration]
...
> cc -Werror -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels 
> -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
> -Wold-style-definition -Wtype-limits -fstack-protector-all 
> -I/usr/include/libpng16 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c 
> -m64 -g -lncursesw
> config-temp/qemu-conf.c: In function ‘main’:
> config-temp/qemu-conf.c:13:3: error: implicit declaration of function 
> ‘addwstr’ [-Werror=implicit-function-declaration]
>addwstr(L"wide chars\n");
>^
> config-temp/qemu-conf.c:13:3: error: nested extern declaration of ‘addwstr’ 
> [-Werror=nested-externs]
> config-temp/qemu-conf.c:14:3: error: implicit declaration of function 
> ‘addnwstr’ [-Werror=implicit-function-declaration]
>addnwstr(, 1);
>^
> config-temp/qemu-conf.c:14:3: error: nested extern declaration of ‘addnwstr’ 
> [-Werror=nested-externs]
> cc1: all warnings being treated as errors

These are expected

But doesn't configure also try with -DCONFIG_CURSESW_H?  Does it really
stop here?

Samuel

Re: [Qemu-devel] [PULL 6/6] curses: Use cursesw instead of curses

[Samuel Thibault]

Cornelia Huck, on Mon 31 Oct 2016 13:48:01 +0100, wrote:
> On Mon, 31 Oct 2016 13:39:30 +0100
> Samuel Thibault <samuel.thiba...@gnu.org> wrote:
> 
> > Cornelia Huck, on Mon 31 Oct 2016 13:08:06 +0100, wrote:
> > > You mean in configure, right? Including cursesw.h in the test program
> > > gets configure going again.
> > 
> > Could you try the attached patch which fixes both configure and
> > ui/curses.c?
> 
> Sadly, this does not fix it for me. I get the same errors in the
> configure test build as before.

Could you post the config.log so we get a better view at what is going
wrong?

Thanks,
Samuel

Re: [Qemu-devel] [PULL 6/6] curses: Use cursesw instead of curses

[Samuel Thibault]

Cornelia Huck, on Mon 31 Oct 2016 13:08:06 +0100, wrote:
> You mean in configure, right? Including cursesw.h in the test program
> gets configure going again.

Could you try the attached patch which fixes both configure and
ui/curses.c?

Thanks,
Samuel
diff --git a/configure b/configure
index f83cdf8..bae01f0 100755
--- a/configure
+++ b/configure
@@ -2920,13 +2920,17 @@ if test "$curses" != "no" ; then
 curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
 curses_lib_list="$($pkg_config --libs ncurses 2>/dev/null):-lpdcurses"
   else
-curses_inc_list="$($pkg_config --cflags ncursesw 2>/dev/null):"
+curses_inc_list="$($pkg_config --cflags ncursesw 
2>/dev/null):-DCONFIG_CURSESW_H:"
 curses_lib_list="$($pkg_config --libs ncursesw 
2>/dev/null):-lncursesw:-lcursesw"
   fi
   curses_found=no
   cat > $TMPC << EOF
 #include 
+#ifdef CONFIG_CURSESW_H
+#include 
+#else
 #include 
+#endif
 #include 
 int main(void) {
   const char *s = curses_version();
@@ -2949,6 +2953,9 @@ EOF
 break
   fi
 done
+if test "$curses_found" = yes ; then
+  break
+fi
   done
   unset IFS
   if test "$curses_found" = "yes" ; then
diff --git a/ui/curses.c b/ui/curses.c
index 2e132a7..cb61073 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -22,7 +22,11 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#ifdef CONFIG_CURSESW_H
+#include 
+#else
 #include 
+#endif
 
 #ifndef _WIN32
 #include 

Re: [Qemu-devel] [PULL 6/6] curses: Use cursesw instead of curses

[Samuel Thibault]

Cornelia Huck, on Mon 31 Oct 2016 13:08:06 +0100, wrote:
> On Mon, 31 Oct 2016 13:01:59 +0100
> Samuel Thibault <samuel.thiba...@gnu.org> wrote:
> 
> > Cornelia Huck, on Mon 31 Oct 2016 12:45:30 +0100, wrote:
> > > config-temp/qemu-conf.c:9:3: warning: implicit declaration of function 
> > > ‘addwstr’ [-Wimplicit-function-declaration]
> > 
> > Bleh.
> > 
> > Could you try to replace #include  with #include 
> > in ui/curses.c, to see whether that fixes the missing declaration on
> > that system?  We could be trying both headers to look for the wide
> > functions and include the one which works.
> 
> You mean in configure, right?

Ah, sorry, I read too fast, I thought what you had was a build failure,
not a configuration failure.

> Including cursesw.h in the test program gets configure going again.

Ok, that makes sense, thanks.

Samuel

Re: [Qemu-devel] [PULL 6/6] curses: Use cursesw instead of curses

[Samuel Thibault]

Samuel Thibault, on Mon 31 Oct 2016 13:01:59 +0100, wrote:
> Cornelia Huck, on Mon 31 Oct 2016 12:45:30 +0100, wrote:
> > > From: Samuel Thibault <samuel.thiba...@ens-lyon.org>
> > > 
> > > Use ncursesw package instead of curses on non-mingw, and check a few
> > > functions.
> > > Also take cflags from pkg-config, since cursesw headers may be in a
> > > separate, non-default directory.
> > > 
> > > Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
> > > Message-id: 20161015195308.20473-3-samuel.thiba...@ens-lyon.org
> > > Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> > > ---
> > >  configure | 29 -
> > >  1 file changed, 20 insertions(+), 9 deletions(-)
> > 
> > This seems to break configure on one of the systems I use (which may or
> > may not have a broken setup). SLES12SP1 (s390x) without curses in the
> > output of pkg-config --list-all, but headers seem to be present (? --
> > I'm not the admin). Other systems (Fedora and Ubuntu) are fine.
> 
> > config-temp/qemu-conf.c:9:3: warning: implicit declaration of function 
> > ‘addwstr’ [-Wimplicit-function-declaration]
> 
> We could be trying both headers to look for the wide
> functions and include the one which works.

I'm however surprised that this didn't get catched by configure, we do
look for addwstr there. Could you also post the config.log?

Thanks,
Samuel

Re: [Qemu-devel] [PULL 6/6] curses: Use cursesw instead of curses

[Samuel Thibault]

Cornelia Huck, on Mon 31 Oct 2016 12:45:30 +0100, wrote:
> > From: Samuel Thibault <samuel.thiba...@ens-lyon.org>
> > 
> > Use ncursesw package instead of curses on non-mingw, and check a few
> > functions.
> > Also take cflags from pkg-config, since cursesw headers may be in a
> > separate, non-default directory.
> > 
> > Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
> > Message-id: 20161015195308.20473-3-samuel.thiba...@ens-lyon.org
> > Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> > ---
> >  configure | 29 -
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> This seems to break configure on one of the systems I use (which may or
> may not have a broken setup). SLES12SP1 (s390x) without curses in the
> output of pkg-config --list-all, but headers seem to be present (? --
> I'm not the admin). Other systems (Fedora and Ubuntu) are fine.

> config-temp/qemu-conf.c:9:3: warning: implicit declaration of function 
> ‘addwstr’ [-Wimplicit-function-declaration]

Bleh.

Could you try to replace #include  with #include 
in ui/curses.c, to see whether that fixes the missing declaration on
that system?  We could be trying both headers to look for the wide
functions and include the one which works.

Samuel

Re: [Qemu-devel] [PATCH 3/3] Move getting XWindow ID from baum driver to graphical backend

[Samuel Thibault]

Gerd Hoffmann, on Wed 26 Oct 2016 12:17:44 +0200, wrote:
> > +/* All consoles share the same window */
> 
> No.  That is the default setup, but try "View / Detach tab".  Window ID
> changing at runtime ...

So we would need to make baum register for notification of Window ID
change.

It could be a mere

typedef void QemuConsoleWindowIDListener(void);
qemu_console_window_id_add_listener(QemuConsoleWindowIDListener listener);
qemu_console_window_id_remove_listener(QemuConsoleWindowIDListener listener);

that adds/removes the listener to a list to be called when
qemu_console_set_window_id is called.

Or we could generalize a bit: 

typedef void QemuConsoleConfigListener(void);
qemu_console_config_add_listener(QemuConsoleConfigListener listener);
qemu_console_config_remove_listener(QemuConsoleConfigListener listener);

Or even more generalized:

struct QemuConsoleListener {
  void (*window_id)(void);
};
typedef struct QemuConsoleListener QemuConsoleListener;
qemu_console_add_listener(QemuConsoleListener *listener);
qemu_console_remove_listener(QemuConsoleListener *listener);

What would be preferrable?

Samuel

[Qemu-devel] [PATCHv2 0/3] Move getting XWindow ID from baum driver to graphical backend

[Samuel Thibault]

Hello,

This is a respin of moving getting XWindow ID from baum to actual
graphical backends.  This only contains the new API, the move
of existing code to the new API, and the addition of support for
SDL2. Gtk will need more discussion and work.

Samuel

Samuel Thibault (3):
  console: add API to get underlying gui window ID
  console: move window ID code from baum to sdl
  sdl2: set window ID

 backends/baum.c  | 25 +++--
 include/ui/console.h |  3 +++
 ui/console.c | 15 +++
 ui/sdl.c | 25 +
 ui/sdl2.c|  7 +++
 5 files changed, 53 insertions(+), 22 deletions(-)

-- 
2.10.1

[Qemu-devel] [PATCH 2/3] console: move window ID code from baum to sdl

[Samuel Thibault]

This moves the SDL bits for window ID from the baum driver to SDL, as
well as fixing the build for non-X11.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 backends/baum.c | 25 +++--
 ui/sdl.c| 25 +
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/backends/baum.c b/backends/baum.c
index b92369d..5d7d27c 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -27,12 +27,10 @@
 #include "sysemu/char.h"
 #include "qemu/timer.h"
 #include "hw/usb.h"
+#include "ui/console.h"
 #include 
 #include 
 #include 
-#ifdef CONFIG_SDL
-#include 
-#endif
 
 #if 0
 #define DPRINTF(fmt, ...) \
@@ -227,11 +225,6 @@ static const uint8_t nabcc_translation[2][256] = {
 /* The guest OS has started discussing with us, finish initializing BrlAPI */
 static int baum_deferred_init(BaumDriverState *baum)
 {
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-SDL_SysWMinfo info;
-#endif
-#endif
 int tty;
 
 if (baum->deferred_init) {
@@ -243,21 +236,9 @@ static int baum_deferred_init(BaumDriverState *baum)
 return 0;
 }
 
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-memset(, 0, sizeof(info));
-SDL_VERSION();
-if (SDL_GetWMInfo()) {
-tty = info.info.x11.wmwindow;
-} else {
-#endif
-#endif
+tty = qemu_graphic_console_get_window_id();
+if (tty == -1)
 tty = BRLAPI_TTY_DEFAULT;
-#if defined(CONFIG_SDL)
-#if SDL_COMPILEDVERSION < SDL_VERSIONNUM(2, 0, 0)
-}
-#endif
-#endif
 
 if (brlapi__enterTtyMode(baum->brlapi, tty, NULL) == -1) {
 brlapi_perror("baum: brlapi__enterTtyMode");
diff --git a/ui/sdl.c b/ui/sdl.c
index d8cf5bc..7fa3772 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -947,6 +947,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 int flags;
 uint8_t data = 0;
 const SDL_VideoInfo *vi;
+SDL_SysWMinfo info;
 char *filename;
 
 #if defined(__APPLE__)
@@ -1023,5 +1024,29 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 sdl_cursor_hidden = SDL_CreateCursor(, , 8, 1, 0, 0);
 sdl_cursor_normal = SDL_GetCursor();
 
+memset(, 0, sizeof(info));
+SDL_VERSION();
+if (SDL_GetWMInfo()) {
+int i;
+for (i = 0; ; i++) {
+/* All consoles share the same window */
+QemuConsole *con = qemu_console_lookup_by_index(i);
+if (con) {
+#if defined(SDL_VIDEO_DRIVER_X11)
+qemu_console_set_window_id(i, info.info.x11.wmwindow);
+#elif defined(SDL_VIDEO_DRIVER_NANOX) || \
+  defined(SDL_VIDEO_DRIVER_WINDIB) || defined(SDL_VIDEO_DRIVER_DDRAW) || \
+  defined(SDL_VIDEO_DRIVER_GAPI) || \
+  defined(SDL_VIDEO_DRIVER_RISCOS)
+qemu_console_set_window_id(i, (int) info.window);
+#else
+qemu_console_set_window_id(i, info.data);
+#endif
+} else {
+break;
+}
+}
+}
+
 atexit(sdl_cleanup);
 }
-- 
2.10.1

[Qemu-devel] [PATCH 1/3] console: add API to get underlying gui window ID

[Samuel Thibault]

This adds two console functions, qemu_console_set_window_id and
qemu_graphic_console_get_window_id, to let graphical backend record the
window id in the QemuConsole structure, and let the baum driver read it.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 include/ui/console.h |  3 +++
 ui/console.c | 15 +++
 2 files changed, 18 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index e2589e2..cf07e41 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -394,6 +394,9 @@ uint32_t qemu_console_get_head(QemuConsole *con);
 QemuUIInfo *qemu_console_get_ui_info(QemuConsole *con);
 int qemu_console_get_width(QemuConsole *con, int fallback);
 int qemu_console_get_height(QemuConsole *con, int fallback);
+/* Return the low-level window id for the first graphical console */
+int qemu_graphic_console_get_window_id(void);
+void qemu_console_set_window_id(int index, int window_id);
 
 void console_select(unsigned int index);
 void qemu_console_resize(QemuConsole *con, int width, int height);
diff --git a/ui/console.c b/ui/console.c
index ed888e5..aa3c4c7 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -124,6 +124,7 @@ struct QemuConsole {
 int dcls;
 DisplayChangeListener *gl;
 bool gl_block;
+int window_id;
 
 /* Graphic console state.  */
 Object *device;
@@ -273,6 +274,20 @@ void graphic_hw_gl_block(QemuConsole *con, bool block)
 }
 }
 
+int qemu_graphic_console_get_window_id(void)
+{
+if (consoles[0]->console_type == GRAPHIC_CONSOLE) {
+return consoles[0]->window_id;
+}
+return -1;
+}
+
+void qemu_console_set_window_id(int index, int window_id)
+{
+assert(index >= 0 && index < nb_consoles);
+consoles[index]->window_id = window_id;
+}
+
 void graphic_hw_invalidate(QemuConsole *con)
 {
 if (!con) {
-- 
2.10.1

[Qemu-devel] [PATCH 3/3] sdl2: set window ID

[Samuel Thibault]

This uses the console API to record the window ID of the SDL2 windows.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
---
 ui/sdl2.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 30d2a3c..b464f16 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -761,6 +761,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 uint8_t data = 0;
 char *filename;
 int i;
+SDL_SysWMinfo info;
 
 if (no_frame) {
 gui_noframe = 1;
@@ -786,6 +787,8 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 exit(1);
 }
 SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
+memset(, 0, sizeof(info));
+SDL_VERSION();
 
 for (i = 0;; i++) {
 QemuConsole *con = qemu_console_lookup_by_index(i);
@@ -813,6 +816,10 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 #endif
 sdl2_console[i].dcl.con = con;
 register_displaychangelistener(_console[i].dcl);
+
+if (SDL_GetWindowWMInfo(sdl2_console[i].real_window, )) {
+qemu_console_set_window_id(i, info.info.x11.window);
+}
 }
 
 /* Load a 32x32x4 image. White pixels are transparent. */
-- 
2.10.1