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

[Anthony Liguori]

Andreas Färber <address@hidden> writes:

> Am 07.03.2013 17:44, schrieb Anthony Liguori:
>> Andreas Färber <address@hidden> writes:
>> 
>>> Am 07.03.2013 17:27, schrieb Christian Borntraeger:
>>>>> It's a bug in both virtio-ccw that features=0 when get_features is
>>>>> called.  You can also tell this with:
>>>>>
>>>>> [10:02 AM] address@hidden:~/git/qemu/hw/s390x$ grep 
>>>>> DEFINE_VIRTIO_NET_FEATURES *
>>>>> virtio-ccw.c:    DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, 
>>>>> host_features[0]),
>>>>>
>>>>> So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it
>>>>> right.
>>>>
>>>> At least, this patch seems to work. (That also implies, that a transport
>>>> must not hide virtio feature bits).
>>>
>>> To me it indicates that the use of the old qdev property setters is
>>> hiding errors resulting from trying to set not-existing properties.
>>> If we would set the properties in a way that gets us an Error* on
>>> failure like the object_property_set_*() do, we would notice on machine
>>> creation (or device_add).
>> 
>> Hrm, I don't understand your statement.
>> 
>> Can you elaborate?
>
> <afaerber> aliguori_, borntraeger added new qdev static properties as
> bug fix
> <afaerber> aliguori_, I was saying if errors setting such properties
> were not silently ignored, we would notice such bugs earlier
>
> I.e., no new field was added, no new value set, so apparently something
> somewhere is setting some of these properties that are defined via
> virtio-net.h:DEFINE_VIRTIO_NET_FEATURES() using DEFINE_PROP_BIT().

No code is explicitly setting the property.  Each property has a default
value (usually true) and that's how we get the initial non-zero value.

So in the absence of the define, we end up with no properties and no
value for this field.

Regards,

Anthony Liguori
>
> And if code expects these properties to be settable, failing to set them
> should be treated as error and an appropriate API for setting individual
> bits with Error **errp argument should be provided.
>
> Unfortunately I don't see any property matching Alex' VIRTIO_NET_F_MAC,
> so I can't draft a patch for illustration.
>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg