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

[Andreas Färber]

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

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