On Wed, 05 Dec 2018 15:16:27 +0100
Markus Armbruster <arm...@redhat.com> wrote:
> Auger Eric <eric.au...@redhat.com> 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 <ebl...@redhat.com>
> >> Cc: Markus Armbruster <arm...@redhat.com>
> >> Tested-by: Geoffrey McRae <ge...@hostfission.com>
> >> Signed-off-by: Alex Williamson <alex.william...@redhat.com>
> >> ---
> >> 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.
I added the comment because as a casual QAPI contributor it's otherwise
not obvious that bogus user input can't reach that case. Comments are
free.
> Note that I'm not this file's maintainer.
get_maintainer.pl says there is no maintainer here and references:
"Michael S. Tsirkin" <m...@redhat.com> (commit_signer:3/4=75%)
"Marc-André Lureau" <marcandre.lur...@redhat.com> (commit_signer:2/4=50%)
Markus Armbruster <arm...@redhat.com> (commit_signer:1/4=25%)
"Philippe Mathieu-Daudé" <f4...@amsat.org> (commit_signer:1/4=25%)
Paolo Bonzini <pbonz...@redhat.com> (commit_signer:1/4=25%)
CC'ing those folks. Unless anyone expresses a strong opinion or trend
towards using g_assert_not_reached(), I'll stick with Markus' style to
only use it in files where g_assert() is already present... or at least
claim that's why I didn't use it ;) Thanks,
Alex
