Forgot to CC: qemu-devel (sorry) - thanks to Markus for the hint.
So let's repeat it here:
On 14/11/2023 10.31, Daniel P. Berrangé wrote:
On Tue, Nov 14, 2023 at 10:05:37AM +0100, Thomas Huth wrote:
QOM names currently don't have any enforced naming rules. This can
be problematic, e.g. when they are used on the command line for
the "-device" option (where the comma is used to separate properties).
To avoid that such problematic type names come in again, let's
disallow them now by adding an g_assert() during the type registration.
Signed-off-by: Thomas Huth <[email protected]>
---
Based-on: <[email protected]>
(without Markus' patches, the g_assert() triggers with the current
code base)
See discussion here:
https://lore.kernel.org/qemu-devel/[email protected]/
Questions: Should we disallow other characters, too? Slash and
backslash maybe (since they can cause trouble with module names)?
Dot and colon would maybe be good candidates, too, but they seem
to be in wide use already, so these don't really seem to be
feasible...
There's two questions.
* What should we enforce today
* What should we ideally enforce in future
Ideally the answers would be the same, but getting there will
almost certainly require some cleanup first.
Given that we can now define QOM types using QAPI, I feel we
preserve everyone's sanity by enforcing the same rules for
QOM and QAPI type naming. IOW
All QOM type names must begin with a letter, and contain
only ASCII letters, digits, hyphen, and underscore.
is the answer for the second question.
In terms of what we can enforce today, we can block ',',
but we can't block '.' without some cleanup, and possibly
the same for ':'. Can we assume we don't have any other
non-alphanumeric chars used ?
If so, I think that today we we could probably get away with
saying:
All QOM type names must begin with a letter, and contain
only ASCII letters, digits, hyphen, underscore, period
and colon. Usage of period and colon is deprecated.
qom/object.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/qom/object.c b/qom/object.c
index 95c0dc8285..85461ab0d2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -113,6 +113,8 @@ static TypeImpl *type_new(const TypeInfo *info)
abort();
}
+ g_assert(!strchr(info->name, ','));
So with helpers:
#define QOM_VALID_TYPECHARS
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLNMOPQRSTUVWXYZ0123456789-_.:"
static bool qom_valid_typename(const char *name) {
return name[0] != '\0' &&
g_ascii_isalpha(name[0]) &&
strspn(name, QOM_VALID_TYPECHARS) == strlen(name));
}
The check would then become
g_assert(qom_valid_typename(info->name));
if (strstr(info->name, '.') ||
strstr(info->name, ':')) {
warn_report("Usage of '.' and ':', in type name %s is deprecated",
info->name)
}
The warn_report could be a bit annoying if we the unusual type names are
something we're going to hit frequently ?
With regards,
Daniel
I've added a debug printf, and seems like we register a lot of types like:
cfi.pflash01
virt-2.6-machine::hotplug-handler
aspeed.i2c.slave::vmstate-if
pc-i440fx-3.0-machine::nmi
... just to name some few. So I don't think it is feasible to use the
warn_report here.
Thomas
