Re: [PATCH v2 6/9] qapi: Generalize command policy checking
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(a)redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd(a)redhat.com>
> Acked-by: John Snow <jsnow(a)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.