diff mbox

[PULL,00/13] vhost, virtio, pci, pxe

Message ID CAFEAcA_wOnQmgc+ujGBF+RUSp7-jH5UkbkrtHzcxCqPVtqn0fA@mail.gmail.com
State New
Headers show

Commit Message

Peter Maydell Feb. 19, 2016, 12:09 p.m. UTC 
On 19 February 2016 at 08:00, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit a5af12871fd4601c44f08d9e49131e9ca13ef102:
>
>   Merge remote-tracking branch 'remotes/sstabellini/tags/xen-2016-02-12' into staging (2016-02-12 17:36:12 +0000)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to a28c393cc261afeb4863b05d7c33c2a5fc55ef38:
>
>   tests/vhost-user-bridge: add scattering of incoming packets (2016-02-18 17:42:05 +0200)
>
> ----------------------------------------------------------------
> vhost, virtio, pci, pxe
>
> Fixes all over the place.
> New tests for pxe.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Hi. With this pullreq I now see new errors in the clang runtime
sanitizer config:

GTESTER check-qtest-i386
/home/petmay01/linaro/qemu-for-merges/slirp/slirp.c:751:13: runtime
error: index 1562 out of bounds for type 'char [1]'
/home/petmay01/linaro/qemu-for-merges/slirp/slirp.c:751:13: runtime
error: index 1562 out of bounds for type 'char [1]'

I think these are "we started testing something new and so the
sanitizer now warns about an old problem" issues. Still, it would
be nice to avoid introducing new sanitizer complaints into the
'make check' run if we can.

We can do that, I think, by updating slirp to use C99 flexible
arrays:


[compile-tested, but need to audit the slirp code to check that
it doesn't care that m_dat and m_ext no longer have the same
offset within the struct... I'm assuming we don't care about
the fact we've used an extra 4 bytes of struct.]

However that then needs us to add a new disable-warning flag
-Wno-gnu-variable-sized-type-not-at-end, because slirp.h uses
these 'struct mbuf's embedded inside another structure, and that
is a GNU extension, which clang will default to complaining about.

So I guess I'll apply this pullreq for now and we can look
at improving the SLIRP code to be sanitizer-clean later.

thanks
-- PMM

Comments

Samuel Thibault Feb. 19, 2016, 12:41 p.m. UTC | #1 
Hello,

Peter Maydell, on Fri 19 Feb 2016 12:09:17 +0000, wrote:
> We can do that, I think, by updating slirp to use C99 flexible
> arrays:

Indeed.

> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
> index 38fedf4..ef5a4f7 100644
> --- a/slirp/mbuf.h
> +++ b/slirp/mbuf.h
> @@ -81,11 +81,9 @@ struct mbuf {
>         Slirp *slirp;
>         bool    resolution_requested;
>         uint64_t expiration_date;
> +       char    *m_ext;
>         /* start of dynamic buffer area, must be last element */
> -       union {
> -               char    m_dat[1]; /* ANSI don't like 0 sized arrays */
> -               char    *m_ext;
> -       };
> +       char    m_dat[];
>  };
> 
>  #define ifq_prev m_prev
> 
> [compile-tested, but need to audit the slirp code to check that
> it doesn't care that m_dat and m_ext no longer have the same
> offset within the struct...

I believe that's true. It needs to be cross-checked though.

> I'm assuming we don't care about
> the fact we've used an extra 4 bytes of struct.]

That's probably fine, yes.

> However that then needs us to add a new disable-warning flag
> -Wno-gnu-variable-sized-type-not-at-end, because slirp.h uses
> these 'struct mbuf's embedded inside another structure,

Uh. It does so for not so useful reasons, it actually really needs a list head
(i.e. a struct quehead), that could probably be reworked.

Samuel
Peter Maydell Feb. 19, 2016, 2:17 p.m. UTC | #2 
On 19 February 2016 at 12:09, Peter Maydell <peter.maydell@linaro.org> wrote:
> So I guess I'll apply this pullreq for now and we can look
> at improving the SLIRP code to be sanitizer-clean later.

Now applied, thanks.

-- PMM
Samuel Thibault March 23, 2016, 12:05 a.m. UTC | #3 
Hello Peter,

Peter Maydell, on Fri 19 Feb 2016 12:09:17 +0000, wrote:
> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
> index 38fedf4..ef5a4f7 100644
> --- a/slirp/mbuf.h
> +++ b/slirp/mbuf.h
> @@ -81,11 +81,9 @@ struct mbuf {
>         Slirp *slirp;
>         bool    resolution_requested;
>         uint64_t expiration_date;
> +       char    *m_ext;
>         /* start of dynamic buffer area, must be last element */
> -       union {
> -               char    m_dat[1]; /* ANSI don't like 0 sized arrays */
> -               char    *m_ext;
> -       };
> +       char    m_dat[];
>  };
> 
>  #define ifq_prev m_prev

Could you give your Signed-off-by on this?

Samuel
Peter Maydell March 23, 2016, 12:43 p.m. UTC | #4 
On 23 March 2016 at 00:05, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Hello Peter,
>
> Peter Maydell, on Fri 19 Feb 2016 12:09:17 +0000, wrote:
>> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
>> index 38fedf4..ef5a4f7 100644
>> --- a/slirp/mbuf.h
>> +++ b/slirp/mbuf.h
>> @@ -81,11 +81,9 @@ struct mbuf {
>>         Slirp *slirp;
>>         bool    resolution_requested;
>>         uint64_t expiration_date;
>> +       char    *m_ext;
>>         /* start of dynamic buffer area, must be last element */
>> -       union {
>> -               char    m_dat[1]; /* ANSI don't like 0 sized arrays */
>> -               char    *m_ext;
>> -       };
>> +       char    m_dat[];
>>  };
>>
>>  #define ifq_prev m_prev
>
> Could you give your Signed-off-by on this?

Sure; Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
though as I say I only compile tested it...

-- PMM
Samuel Thibault March 23, 2016, 12:45 p.m. UTC | #5 
Peter Maydell, on Wed 23 Mar 2016 12:43:44 +0000, wrote:
> On 23 March 2016 at 00:05, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Peter Maydell, on Fri 19 Feb 2016 12:09:17 +0000, wrote:
> >> diff --git a/slirp/mbuf.h b/slirp/mbuf.h
> >> index 38fedf4..ef5a4f7 100644
> >> --- a/slirp/mbuf.h
> >> +++ b/slirp/mbuf.h
> >> @@ -81,11 +81,9 @@ struct mbuf {
> >>         Slirp *slirp;
> >>         bool    resolution_requested;
> >>         uint64_t expiration_date;
> >> +       char    *m_ext;
> >>         /* start of dynamic buffer area, must be last element */
> >> -       union {
> >> -               char    m_dat[1]; /* ANSI don't like 0 sized arrays */
> >> -               char    *m_ext;
> >> -       };
> >> +       char    m_dat[];
> >>  };
> >>
> >>  #define ifq_prev m_prev
> >
> > Could you give your Signed-off-by on this?
> 
> Sure; Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> though as I say I only compile tested it...

Sure, but I reviewed it and tested it :)

Samuel
diff mbox

Patch

diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index 38fedf4..ef5a4f7 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -81,11 +81,9 @@  struct mbuf {
        Slirp *slirp;
        bool    resolution_requested;
        uint64_t expiration_date;
+       char    *m_ext;
        /* start of dynamic buffer area, must be last element */
-       union {
-               char    m_dat[1]; /* ANSI don't like 0 sized arrays */
-               char    *m_ext;
-       };
+       char    m_dat[];
 };

 #define ifq_prev m_prev