Re: [PATCH v2 6/9] qapi: Generalize command policy checking

[Philippe Mathieu-Daudé]

On 10/29/21 17:28, Eric Blake wrote:
> On Thu, Oct 28, 2021 at 12:25:17PM +0200, Markus Armbruster wrote:
>> The code to check command policy can see special feature flag
>> 'deprecated' as command flag QCO_DEPRECATED.  I want to make feature
>> flag 'unstable' visible there as well, so I can add policy for it.
>>
>> To let me make it visible, add member @special_features (a bitset of
>> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
>> through qmp_register_command().  Then replace "QCO_DEPRECATED in
>> @flags" by QAPI_DEPRECATED in @special_features", and drop
>> QCO_DEPRECATED.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Acked-by: John Snow <jsnow@redhat.com>
>> ---
> 
>> +++ b/qapi/qmp-dispatch.c
>> @@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject 
>> *request,
>>                    "The command %s has not been found", command);
>>          goto out;
>>      }
>> -    if (cmd->options & QCO_DEPRECATED) {
>> +    if (cmd->special_features & 1u << QAPI_DEPRECATED) {
> 
> I admit having to check the C operator precedence table when reading

This doesn't seem a good use of (y)our time. Using a pair of parenthesis
is simpler.

I expect in a not far future that compilers emit a warning for this.

> this (<< is higher than &); if writing it myself, I would probably
> have used explicit () to avoid reviewer confusion, but what you have
> is correct.  (After grepping for ' & 1.*<<' and ' & (1.*<<', it looks
> like authors using explicit precedence happens more often, but that
> there are other instances in the code base relying on implicit
> precedence.)

$ git grep -E ' & [0-9a-zA-Z_]+ <<'
hw/dma/pl330.c:997:    if (~ch->parent->inten & ch->parent->ev_status &
1 << ev_id) {
hw/dma/xlnx-zynq-devcfg.c:198:        if (s->regs[R_LOCK] & 1 << i) {
hw/intc/grlib_irqmp.c:144:        if (s->broadcast & 1 << irq) {
hw/net/fsl_etsec/rings.c:491:    if (etsec->regs[RSTAT].value & 1 << (23
- ring_nbr)) {
hw/net/virtio-net.c:748:            (n->host_features & 1ULL <<
VIRTIO_NET_F_MTU)) {
hw/pci-host/mv64361.c:812:                if ((ch & 0xff << i) && !(val
& 0xff << i)) {
hw/pci-host/mv64361.c:858:        if (s->gpp_int_level && !(val & 0xff
<< b)) {
hw/ssi/xilinx_spi.c:123:        qemu_set_irq(s->cs_lines[i],
!(~s->regs[R_SPISSR] & 1 << i));
hw/ssi/xilinx_spips.c:441:            r[idx[!d]] |= x[idx[d]] & 1 <<
bit[d] ? 1 << bit[!d] : 0;
target/s390x/cpu_features.c:56:                if (init[i] & 1ULL << j) {
tests/qtest/bios-tables-test.c:209:    if (!(val & 1UL << 20 /*
HW_REDUCED_ACPI */)) {

Not that many.