qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features
Date: Thu, 07 Feb 2013 09:53:15 -0600
User-agent: Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu)

Laszlo Ersek <address@hidden> writes:

> On 02/07/13 09:55, Michael S. Tsirkin wrote:
>> On Tue, Feb 05, 2013 at 05:47:16PM -0600, Jesse Larrew wrote:
>>> Currently, the config size for virtio devices is hard coded. When a new
>>> feature is added that changes the config size, drivers that assume a static
>>> config size will break. For purposes of backward compatibility, there needs
>>> to be a way to inform drivers of the config size needed to accommodate the
>>> set of features enabled.
>>>
>>> Signed-off-by: Jesse Larrew <address@hidden>
>>> ---
>>>  hw/virtio-net.c | 44 ++++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>>> index f1c2884..8f521b3 100644
>>> --- a/hw/virtio-net.c
>>> +++ b/hw/virtio-net.c
>>> @@ -73,8 +73,31 @@ typedef struct VirtIONet
>>>      int multiqueue;
>>>      uint16_t max_queues;
>>>      uint16_t curr_queues;
>>> +    int config_size;
>>>  } VirtIONet;
>>>  
>>> +/*
>>> + * Calculate the number of bytes up to and including the given 'field' of
>>> + * 'container'.
>>> + */
>>> +#define endof(container, field) \
>>> +    ((intptr_t)(&(((container *)0)->field)+1))
>> 
>> Hmm I'm worried whether it's legal to take a pointer to a
>> field in a packed structure.
>
> Yes, it is.
>
> (a) Padding inside structures is in the compiler's jurisdiction (except
> before the first member, where it must be 0 -- C99 6.7.2.1p13). If you
> tell the compiler to pack members, then it's your responsibility to make
> sure no problems will come from accesses to non-naturally aligned fields.
>
> (b) If the member is not a bitfield, you can take its address (C99
> 6.5.3.2p1). Since these unaligned fields are accessed *anyway* without
> problems in practice, it's safe to dereference their addresses in practice.
>
> IOW, purely by these fields being unaligned and already well-accessible
> in practice, you don't commit a greater crime by taking their addresses.
>
>
> Furthermore, taking the pointer to "one past" &field, ie. (&field + 1),
> is safe, provided &field itself is well-defined. This comes from two
> bits in the ISO C standard, sloppily paraphrased:
>
> - Given an array with N elements, it's allowed to form a pointer to one
> past the last element in it. IOW, &array[N], or (array + N), or
> (&array[0] + N). You can't dereference it, but the pointer itself is
> valid. (C99 6.5.6p8-9.)

Indeed.  And due to the associativity of addition, it's also equivalent
to &(N + array) or even &N[array] which is my all time favorite weird C
syntax :-)

> - For the purposes of pointer arithmetic, (&x) works like (&x_array[0]),
> where x is an object of type X, outside of an array, and x_array has
> element type X and N=1 element. (C99 6.5.6p7.)
>
> Since you're allowed to evaluate ((&x_array[0]) + 1), you're also
> allowed to eval ((&x) + 1).
>
>
> Anyway the above expression could be reworked for more portability with
> offsetof and sizeof.  As-is, it contains a pointer-to-non-void to
> intptr_t conversion; what's more, with the resultant integer value meant
> to be used numerically later on (ie. not for converting it back to
> (void*), C99 7.18.1.4p). (The use of the null pointer is valid, the
> dereference is not evaluated because it's the operand of &, C99
> 6.5.2.3p4 and 6.5.3.2p3.)
>
> Instead, what about
>
> #define endof(container, field) \
>     (offsetof(container, field) + sizeof ((container *)0)->field)

That's fine too and probably a lot easier to understand.

Regards,

Anthony Liguori

>
> Laszlo




reply via email to

[Prev in Thread] Current Thread [Next in Thread]