[Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails

Greg Kurz posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/148976155089.11859.11860739290129101204.stgit@bahia
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
hw/9pfs/9p-proxy.c |   24 +++++++++++++-----------
hw/9pfs/9p-proxy.h |   10 ++++++++--
2 files changed, 21 insertions(+), 13 deletions(-)
[Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails
Posted by Greg Kurz 7 years, 1 month ago
Replies from the virtfs proxy are made up of a fixed-size header (8 bytes)
and a payload of variable size (maximum 64kb). When receiving a reply,
the proxy backend first reads the whole header and then unmarshals it.
If the header is okay, it then does the same operation with the payload.

Since the proxy backend uses a pre-allocated buffer which has enough room
for a header and the maximum payload size, marshalling should never fail
with fixed size arguments. Any error here is likely to result from a more
serious corruption in QEMU and we'd better dump core right away.

This patch adds error checks where they are missing and converts the
associated error paths into assertions.

Note that we don't want to use sizeof() in the checks since the value
we want to use depends on the format rather than the size of the buffer.
Short marshal formats can be handled with numerical values. Size macros
are introduced for bigger ones (ProxyStat and ProxyStatFS).

This should also address Coverity's complaints CID 1348519 and CID 1348520,
about not always checking the return value of proxy_unmarshal().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros
    - updated changelog
---
 hw/9pfs/9p-proxy.c |   24 +++++++++++++-----------
 hw/9pfs/9p-proxy.h |   10 ++++++++--
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index f4aa7a9d70f8..363bea66035e 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
         return retval;
     }
     reply->iov_len = PROXY_HDR_SZ;
-    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
+    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
+    assert(retval == 8);
     /*
      * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and
      * return -ENOBUFS
@@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
     if (header.type == T_ERROR) {
         int ret;
         ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
-        if (ret < 0) {
-            *status = ret;
-        }
+        assert(ret == 4);
         return 0;
     }
 
     switch (type) {
     case T_LSTAT: {
         ProxyStat prstat;
+        QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ);
         retval = proxy_unmarshal(reply, PROXY_HDR_SZ,
                                  "qqqdddqqqqqqqqqq", &prstat.st_dev,
                                  &prstat.st_ino, &prstat.st_nlink,
@@ -213,11 +213,13 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
                                  &prstat.st_atim_sec, &prstat.st_atim_nsec,
                                  &prstat.st_mtim_sec, &prstat.st_mtim_nsec,
                                  &prstat.st_ctim_sec, &prstat.st_ctim_nsec);
+        assert(retval == PROXY_STAT_SZ);
         prstat_to_stat(response, &prstat);
         break;
     }
     case T_STATFS: {
         ProxyStatFS prstfs;
+        QEMU_BUILD_BUG_ON(sizeof(prstfs) != PROXY_STATFS_SZ);
         retval = proxy_unmarshal(reply, PROXY_HDR_SZ,
                                  "qqqqqqqqqqq", &prstfs.f_type,
                                  &prstfs.f_bsize, &prstfs.f_blocks,
@@ -225,6 +227,7 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
                                  &prstfs.f_files, &prstfs.f_ffree,
                                  &prstfs.f_fsid[0], &prstfs.f_fsid[1],
                                  &prstfs.f_namelen, &prstfs.f_frsize);
+        assert(retval == PROXY_STATFS_SZ);
         prstatfs_to_statfs(response, &prstfs);
         break;
     }
@@ -246,7 +249,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
         break;
     }
     case T_GETVERSION:
-        proxy_unmarshal(reply, PROXY_HDR_SZ, "q", response);
+        retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "q", response);
+        assert(retval == 8);
         break;
     default:
         return -1;
@@ -274,18 +278,16 @@ static int v9fs_receive_status(V9fsProxy *proxy,
         return retval;
     }
     reply->iov_len = PROXY_HDR_SZ;
-    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
-    if (header.size != sizeof(int)) {
-        *status = -ENOBUFS;
-        return 0;
-    }
+    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
+    assert(retval == 8);
     retval = socket_read(proxy->sockfd,
                          reply->iov_base + PROXY_HDR_SZ, header.size);
     if (retval < 0) {
         return retval;
     }
     reply->iov_len += header.size;
-    proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
+    retval = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
+    assert(retval == 4);
     return 0;
 }
 
diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h
index b84301d001b0..918c45016a3c 100644
--- a/hw/9pfs/9p-proxy.h
+++ b/hw/9pfs/9p-proxy.h
@@ -79,7 +79,10 @@ typedef struct {
     uint64_t st_mtim_nsec;
     uint64_t st_ctim_sec;
     uint64_t st_ctim_nsec;
-} ProxyStat;
+} QEMU_PACKED ProxyStat;
+
+#define PROXY_STAT_SZ \
+    (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)
 
 typedef struct {
     uint64_t f_type;
@@ -92,5 +95,8 @@ typedef struct {
     uint64_t f_fsid[2];
     uint64_t f_namelen;
     uint64_t f_frsize;
-} ProxyStatFS;
+} QEMU_PACKED ProxyStatFS;
+
+#define PROXY_STATFS_SZ (8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 * 2 + 8 + 8)
+
 #endif


Re: [Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails
Posted by Daniel P. Berrange 7 years, 1 month ago
On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote:
> Replies from the virtfs proxy are made up of a fixed-size header (8 bytes)
> and a payload of variable size (maximum 64kb). When receiving a reply,
> the proxy backend first reads the whole header and then unmarshals it.
> If the header is okay, it then does the same operation with the payload.
> 
> Since the proxy backend uses a pre-allocated buffer which has enough room
> for a header and the maximum payload size, marshalling should never fail
> with fixed size arguments. Any error here is likely to result from a more
> serious corruption in QEMU and we'd better dump core right away.
> 
> This patch adds error checks where they are missing and converts the
> associated error paths into assertions.
> 
> Note that we don't want to use sizeof() in the checks since the value
> we want to use depends on the format rather than the size of the buffer.
> Short marshal formats can be handled with numerical values. Size macros
> are introduced for bigger ones (ProxyStat and ProxyStatFS).
> 
> This should also address Coverity's complaints CID 1348519 and CID 1348520,
> about not always checking the return value of proxy_unmarshal().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros
>     - updated changelog
> ---
>  hw/9pfs/9p-proxy.c |   24 +++++++++++++-----------
>  hw/9pfs/9p-proxy.h |   10 ++++++++--
>  2 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index f4aa7a9d70f8..363bea66035e 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>          return retval;
>      }
>      reply->iov_len = PROXY_HDR_SZ;
> -    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> +    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> +    assert(retval == 8);
>      /*
>       * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and
>       * return -ENOBUFS
> @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>      if (header.type == T_ERROR) {
>          int ret;
>          ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
> -        if (ret < 0) {
> -            *status = ret;
> -        }
> +        assert(ret == 4);
>          return 0;
>      }
>  
>      switch (type) {
>      case T_LSTAT: {
>          ProxyStat prstat;
> +        QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ);

I'd just put this assert at the struct declaration

..snip...

> diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h
> index b84301d001b0..918c45016a3c 100644
> --- a/hw/9pfs/9p-proxy.h
> +++ b/hw/9pfs/9p-proxy.h
> @@ -79,7 +79,10 @@ typedef struct {
>      uint64_t st_mtim_nsec;
>      uint64_t st_ctim_sec;
>      uint64_t st_ctim_nsec;
> -} ProxyStat;
> +} QEMU_PACKED ProxyStat;
> +
> +#define PROXY_STAT_SZ \
> +    (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)

eg.

  QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
      (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8));


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails
Posted by Philippe Mathieu-Daudé 7 years, 1 month ago
On 03/17/2017 11:43 AM, Daniel P. Berrange wrote:
> On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote:
>> Replies from the virtfs proxy are made up of a fixed-size header (8 bytes)
>> and a payload of variable size (maximum 64kb). When receiving a reply,
>> the proxy backend first reads the whole header and then unmarshals it.
>> If the header is okay, it then does the same operation with the payload.
>>
>> Since the proxy backend uses a pre-allocated buffer which has enough room
>> for a header and the maximum payload size, marshalling should never fail
>> with fixed size arguments. Any error here is likely to result from a more
>> serious corruption in QEMU and we'd better dump core right away.
>>
>> This patch adds error checks where they are missing and converts the
>> associated error paths into assertions.
>>
>> Note that we don't want to use sizeof() in the checks since the value
>> we want to use depends on the format rather than the size of the buffer.
>> Short marshal formats can be handled with numerical values. Size macros
>> are introduced for bigger ones (ProxyStat and ProxyStatFS).

:) maybe this comment should go somewhere in <hw/9pfs/9p-proxy.h>

>> This should also address Coverity's complaints CID 1348519 and CID 1348520,
>> about not always checking the return value of proxy_unmarshal().
>>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>> ---
>> v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros
>>     - updated changelog
>> ---
>>  hw/9pfs/9p-proxy.c |   24 +++++++++++++-----------
>>  hw/9pfs/9p-proxy.h |   10 ++++++++--
>>  2 files changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
>> index f4aa7a9d70f8..363bea66035e 100644
>> --- a/hw/9pfs/9p-proxy.c
>> +++ b/hw/9pfs/9p-proxy.c
>> @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>>          return retval;
>>      }
>>      reply->iov_len = PROXY_HDR_SZ;
>> -    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>> +    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
>> +    assert(retval == 8);
>>      /*
>>       * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and
>>       * return -ENOBUFS
>> @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
>>      if (header.type == T_ERROR) {
>>          int ret;
>>          ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
>> -        if (ret < 0) {
>> -            *status = ret;
>> -        }
>> +        assert(ret == 4);
>>          return 0;
>>      }
>>
>>      switch (type) {
>>      case T_LSTAT: {
>>          ProxyStat prstat;
>> +        QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ);
>
> I'd just put this assert at the struct declaration
>
> ..snip...
>
>> diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h
>> index b84301d001b0..918c45016a3c 100644
>> --- a/hw/9pfs/9p-proxy.h
>> +++ b/hw/9pfs/9p-proxy.h
>> @@ -79,7 +79,10 @@ typedef struct {
>>      uint64_t st_mtim_nsec;
>>      uint64_t st_ctim_sec;
>>      uint64_t st_ctim_nsec;
>> -} ProxyStat;
>> +} QEMU_PACKED ProxyStat;
>> +
>> +#define PROXY_STAT_SZ \
>> +    (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)
>
> eg.
>
>   QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
>       (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8));

or

     QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
         proxy_unmarshal_calcsize("qqqdddqqqqqqqqqq"));

or eventually stricter using some gcc/clang Statement Expressions:

#define proxy_unmarshal_strict(in_sg, in_sg_sz, offset, fmt, args...) \
     ({ \
         ssize_t retval; \
         retval = v9fs_iov_unmarshal(in_sg, 1, offset, 0, fmt, ##args); \
         QEMU_BUILD_BUG_ON(in_sg_sz != proxy_unmarshal_calcsize(fmt)); \
         retval; \
     })

so we could use:

     retval = proxy_unmarshal_strict(reply, sizeof(reply), 0, "dd",
                                     &header.type, &header.size);

>
>
> Regards,
> Daniel
>

Re: [Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails
Posted by Greg Kurz 7 years, 1 month ago
On Fri, 17 Mar 2017 12:06:39 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 03/17/2017 11:43 AM, Daniel P. Berrange wrote:
> > On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote:  
> >> Replies from the virtfs proxy are made up of a fixed-size header (8 bytes)
> >> and a payload of variable size (maximum 64kb). When receiving a reply,
> >> the proxy backend first reads the whole header and then unmarshals it.
> >> If the header is okay, it then does the same operation with the payload.
> >>
> >> Since the proxy backend uses a pre-allocated buffer which has enough room
> >> for a header and the maximum payload size, marshalling should never fail
> >> with fixed size arguments. Any error here is likely to result from a more
> >> serious corruption in QEMU and we'd better dump core right away.
> >>
> >> This patch adds error checks where they are missing and converts the
> >> associated error paths into assertions.
> >>
> >> Note that we don't want to use sizeof() in the checks since the value
> >> we want to use depends on the format rather than the size of the buffer.
> >> Short marshal formats can be handled with numerical values. Size macros
> >> are introduced for bigger ones (ProxyStat and ProxyStatFS).  
> 
> :) maybe this comment should go somewhere in <hw/9pfs/9p-proxy.h>
> 
> >> This should also address Coverity's complaints CID 1348519 and CID 1348520,
> >> about not always checking the return value of proxy_unmarshal().
> >>
> >> Signed-off-by: Greg Kurz <groug@kaod.org>
> >> ---
> >> v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros
> >>     - updated changelog
> >> ---
> >>  hw/9pfs/9p-proxy.c |   24 +++++++++++++-----------
> >>  hw/9pfs/9p-proxy.h |   10 ++++++++--
> >>  2 files changed, 21 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> >> index f4aa7a9d70f8..363bea66035e 100644
> >> --- a/hw/9pfs/9p-proxy.c
> >> +++ b/hw/9pfs/9p-proxy.c
> >> @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
> >>          return retval;
> >>      }
> >>      reply->iov_len = PROXY_HDR_SZ;
> >> -    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> >> +    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> >> +    assert(retval == 8);
> >>      /*
> >>       * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and
> >>       * return -ENOBUFS
> >> @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
> >>      if (header.type == T_ERROR) {
> >>          int ret;
> >>          ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
> >> -        if (ret < 0) {
> >> -            *status = ret;
> >> -        }
> >> +        assert(ret == 4);
> >>          return 0;
> >>      }
> >>
> >>      switch (type) {
> >>      case T_LSTAT: {
> >>          ProxyStat prstat;
> >> +        QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ);  
> >
> > I'd just put this assert at the struct declaration
> >
> > ..snip...
> >  
> >> diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h
> >> index b84301d001b0..918c45016a3c 100644
> >> --- a/hw/9pfs/9p-proxy.h
> >> +++ b/hw/9pfs/9p-proxy.h
> >> @@ -79,7 +79,10 @@ typedef struct {
> >>      uint64_t st_mtim_nsec;
> >>      uint64_t st_ctim_sec;
> >>      uint64_t st_ctim_nsec;
> >> -} ProxyStat;
> >> +} QEMU_PACKED ProxyStat;
> >> +
> >> +#define PROXY_STAT_SZ \
> >> +    (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)  
> >
> > eg.
> >
> >   QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
> >       (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8));  
> 
> or
> 
>      QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
>          proxy_unmarshal_calcsize("qqqdddqqqqqqqqqq"));
> 

The purpose of QEMU_BUILD_BUG_ON(x) is to break the build if the condition
is false.

It expands to something like:

typedef struct { int:(cond) ? -1 : 1; } qemu_build_bug_on__5 __attribute__((unused));

and causes gcc to fail if cond is 0:

error: negative width in bit-field ‘<anonymous>’

This really only makes sense if cond is constant, otherwise gcc complains with:

error: bit-field ‘<anonymous>’ width not an integer constant

And I can't think of any way of implementing proxy_unmarshal_calcsize() so
that it boils down to a constant at build time. Also, the protocol is stable
and won't ever change: it is ok to compute the sizes once and for all.

> or eventually stricter using some gcc/clang Statement Expressions:
> 
> #define proxy_unmarshal_strict(in_sg, in_sg_sz, offset, fmt, args...) \
>      ({ \
>          ssize_t retval; \
>          retval = v9fs_iov_unmarshal(in_sg, 1, offset, 0, fmt, ##args); \
>          QEMU_BUILD_BUG_ON(in_sg_sz != proxy_unmarshal_calcsize(fmt)); \

This should be an assert() actually, and, again, proxy_unmarshal_calcsize()
cannot be a constant, and I certainly don't want to do anything but comparing
two integers here at runtime.

>          retval; \
>      })
> 
> so we could use:
> 
>      retval = proxy_unmarshal_strict(reply, sizeof(reply), 0, "dd",
>                                      &header.type, &header.size);
> 

I wanted to do something like this initially but I decided not to because
of the reasons exposed above. But if you have a solution, I'll gladly
give a try. :)

Thanks

--
Greg


> >
> >
> > Regards,
> > Daniel
> >  

Re: [Qemu-devel] [PATCH v2] 9pfs: proxy: assert if unmarshal fails
Posted by Greg Kurz 7 years, 1 month ago
On Fri, 17 Mar 2017 14:43:00 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Fri, Mar 17, 2017 at 03:39:10PM +0100, Greg Kurz wrote:
> > Replies from the virtfs proxy are made up of a fixed-size header (8 bytes)
> > and a payload of variable size (maximum 64kb). When receiving a reply,
> > the proxy backend first reads the whole header and then unmarshals it.
> > If the header is okay, it then does the same operation with the payload.
> > 
> > Since the proxy backend uses a pre-allocated buffer which has enough room
> > for a header and the maximum payload size, marshalling should never fail
> > with fixed size arguments. Any error here is likely to result from a more
> > serious corruption in QEMU and we'd better dump core right away.
> > 
> > This patch adds error checks where they are missing and converts the
> > associated error paths into assertions.
> > 
> > Note that we don't want to use sizeof() in the checks since the value
> > we want to use depends on the format rather than the size of the buffer.
> > Short marshal formats can be handled with numerical values. Size macros
> > are introduced for bigger ones (ProxyStat and ProxyStatFS).
> > 
> > This should also address Coverity's complaints CID 1348519 and CID 1348520,
> > about not always checking the return value of proxy_unmarshal().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > v2: - added PROXY_STAT_SZ and PROXY_STATFS_SZ macros
> >     - updated changelog
> > ---
> >  hw/9pfs/9p-proxy.c |   24 +++++++++++++-----------
> >  hw/9pfs/9p-proxy.h |   10 ++++++++--
> >  2 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> > index f4aa7a9d70f8..363bea66035e 100644
> > --- a/hw/9pfs/9p-proxy.c
> > +++ b/hw/9pfs/9p-proxy.c
> > @@ -165,7 +165,8 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
> >          return retval;
> >      }
> >      reply->iov_len = PROXY_HDR_SZ;
> > -    proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> > +    retval = proxy_unmarshal(reply, 0, "dd", &header.type, &header.size);
> > +    assert(retval == 8);
> >      /*
> >       * if response size > PROXY_MAX_IO_SZ, read the response but ignore it and
> >       * return -ENOBUFS
> > @@ -194,15 +195,14 @@ static int v9fs_receive_response(V9fsProxy *proxy, int type,
> >      if (header.type == T_ERROR) {
> >          int ret;
> >          ret = proxy_unmarshal(reply, PROXY_HDR_SZ, "d", status);
> > -        if (ret < 0) {
> > -            *status = ret;
> > -        }
> > +        assert(ret == 4);
> >          return 0;
> >      }
> >  
> >      switch (type) {
> >      case T_LSTAT: {
> >          ProxyStat prstat;
> > +        QEMU_BUILD_BUG_ON(sizeof(prstat) != PROXY_STAT_SZ);  
> 
> I'd just put this assert at the struct declaration
> 
> ..snip...
> 
> > diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h
> > index b84301d001b0..918c45016a3c 100644
> > --- a/hw/9pfs/9p-proxy.h
> > +++ b/hw/9pfs/9p-proxy.h
> > @@ -79,7 +79,10 @@ typedef struct {
> >      uint64_t st_mtim_nsec;
> >      uint64_t st_ctim_sec;
> >      uint64_t st_ctim_nsec;
> > -} ProxyStat;
> > +} QEMU_PACKED ProxyStat;
> > +
> > +#define PROXY_STAT_SZ \
> > +    (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8)  
> 
> eg.
> 
>   QEMU_BUILD_BUG_ON(sizeof(ProxyStat) !=
>       (8 + 8 + 8 + 4 + 4 + 4 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8 + 8));
> 

Makes sense. I'll send a v3.

Thanks.

--
Greg

> 
> Regards,
> Daniel