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

[Laszlo Ersek]

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.)

- 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)

Laszlo