Re: [Qemu-devel] [for-4.0 PATCH v3 3/9] qapi: Define PCIe link speed and width properties

[Markus Armbruster]

Auger Eric <address@hidden> writes:

> Hi Alex,
>
> On 12/4/18 5:26 PM, Alex Williamson wrote:
>> Create properties to be able to define speeds and widths for PCIe
>> links.  The only tricky bit here is that our get and set callbacks
>> translate from the fixed QAPI automagic enums to those we define
>> in PCI code to represent the actual register segment value.
>> 
>> Cc: Eric Blake <address@hidden>
>> Cc: Markus Armbruster <address@hidden>
>> Tested-by: Geoffrey McRae <address@hidden>
>> Signed-off-by: Alex Williamson <address@hidden>
>> ---
>>  hw/core/qdev-properties.c    |  178 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/qdev-properties.h |    8 ++
>>  qapi/common.json             |   42 ++++++++++
>>  3 files changed, 228 insertions(+)
>> 
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 35072dec1ecf..f5ca5b821a79 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -1327,3 +1327,181 @@ const PropertyInfo qdev_prop_off_auto_pcibar = {
>>      .set = set_enum,
>>      .set_default_value = set_default_value_enum,
>>  };
>> +
>> +/* --- PCIELinkSpeed 2_5/5/8/16 -- */
>> +
>> +static void get_prop_pcielinkspeed(Object *obj, Visitor *v, const char 
>> *name,
>> +                                   void *opaque, Error **errp)
>> +{
>> +    DeviceState *dev = DEVICE(obj);
>> +    Property *prop = opaque;
>> +    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
>> +    PCIELinkSpeed speed;
>> +
>> +    switch (*p) {
>> +    case QEMU_PCI_EXP_LNK_2_5GT:
>> +        speed = PCIE_LINK_SPEED_2_5;
>> +        break;
>> +    case QEMU_PCI_EXP_LNK_5GT:
>> +        speed = PCIE_LINK_SPEED_5;
>> +        break;
>> +    case QEMU_PCI_EXP_LNK_8GT:
>> +        speed = PCIE_LINK_SPEED_8;
>> +        break;
>> +    case QEMU_PCI_EXP_LNK_16GT:
>> +        speed = PCIE_LINK_SPEED_16;
>> +        break;
>> +    default:
>> +        /* Unreachable */
>> +        abort();
> nit: g_assert_not_reached() here and below.

In my opinion, g_assert_not_reached() & friends are an overly ornate
reinvention of an old and perfectly adequate wheel.

A long time ago for reasons since forgotten, the maintainers in charge
back then demanded abort() instead of assert(0).  Either is fine with
me.

I tolerate g_assert_not_reached() in files that already use g_assert().
This one doesn't.

In any case, I'd drop the comment.

Note that I'm not this file's maintainer.

[...]