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 >> Reviewed-by: Philippe Mathieu-Daudé >> Acked-by: John Snow >> --- > >> +++ 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.
Re: [PATCH v2 6/9] qapi: Generalize command policy checking
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 > Reviewed-by: Philippe Mathieu-Daudé > Acked-by: John Snow > --- > +++ 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 (<< 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.) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 6/9] qapi: Generalize command policy checking
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 > Reviewed-by: Philippe Mathieu-Daudé > Acked-by: John Snow Reviewed-by: Juan Quintela
[PATCH v2 6/9] qapi: Generalize command policy checking
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 Reviewed-by: Philippe Mathieu-Daudé Acked-by: John Snow --- include/qapi/qmp/dispatch.h | 5 +++-- monitor/misc.c | 6 -- qapi/qmp-dispatch.c | 2 +- qapi/qmp-registry.c | 4 +++- storage-daemon/qemu-storage-daemon.c | 3 ++- scripts/qapi/commands.py | 9 - 6 files changed, 17 insertions(+), 12 deletions(-) diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 0ce88200b9..1e4240fd0d 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -25,7 +25,6 @@ typedef enum QmpCommandOptions QCO_ALLOW_OOB = (1U << 1), QCO_ALLOW_PRECONFIG = (1U << 2), QCO_COROUTINE = (1U << 3), -QCO_DEPRECATED= (1U << 4), } QmpCommandOptions; typedef struct QmpCommand @@ -34,6 +33,7 @@ typedef struct QmpCommand /* Runs in coroutine context if QCO_COROUTINE is set */ QmpCommandFunc *fn; QmpCommandOptions options; +unsigned special_features; QTAILQ_ENTRY(QmpCommand) node; bool enabled; const char *disable_reason; @@ -42,7 +42,8 @@ typedef struct QmpCommand typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList; void qmp_register_command(QmpCommandList *cmds, const char *name, - QmpCommandFunc *fn, QmpCommandOptions options); + QmpCommandFunc *fn, QmpCommandOptions options, + unsigned special_features); const QmpCommand *qmp_find_command(const QmpCommandList *cmds, const char *name); void qmp_disable_command(QmpCommandList *cmds, const char *name, diff --git a/monitor/misc.c b/monitor/misc.c index 3556b177f6..c2d227a07c 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -230,11 +230,13 @@ static void monitor_init_qmp_commands(void) qmp_init_marshal(_commands); -qmp_register_command(_commands, "device_add", qmp_device_add, 0); +qmp_register_command(_commands, "device_add", + qmp_device_add, 0, 0); QTAILQ_INIT(_cap_negotiation_commands); qmp_register_command(_cap_negotiation_commands, "qmp_capabilities", - qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG); + qmp_marshal_qmp_capabilities, + QCO_ALLOW_PRECONFIG, 0); } /* Set the current CPU defined by the user. Callers must hold BQL. */ diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 7e943a0af5..8cca18c891 100644 --- a/qapi/qmp-dispatch.c +++ 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) { switch (compat_policy.deprecated_input) { case COMPAT_POLICY_INPUT_ACCEPT: break; diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c index f78c064aae..485bc5e6fc 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -16,7 +16,8 @@ #include "qapi/qmp/dispatch.h" void qmp_register_command(QmpCommandList *cmds, const char *name, - QmpCommandFunc *fn, QmpCommandOptions options) + QmpCommandFunc *fn, QmpCommandOptions options, + unsigned special_features) { QmpCommand *cmd = g_malloc0(sizeof(*cmd)); @@ -27,6 +28,7 @@ void qmp_register_command(QmpCommandList *cmds, const char *name, cmd->fn = fn; cmd->enabled = true; cmd->options = options; +cmd->special_features = special_features; QTAILQ_INSERT_TAIL(cmds, cmd, node); } diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index 10a1a33761..52cf17e8ac 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -146,7 +146,8 @@ static void init_qmp_commands(void) QTAILQ_INIT(_cap_negotiation_commands); qmp_register_command(_cap_negotiation_commands, "qmp_capabilities", - qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG); + qmp_marshal_qmp_capabilities, + QCO_ALLOW_PRECONFIG, 0); } static int getopt_set_loc(int argc, char **argv, const char *optstring, diff --git a/scripts/qapi/commands.py