Kevin Wolf <kw...@redhat.com> writes: > Am 02.11.2023 um 13:55 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > The 'name' parameter of QOM setters is primarily used to specify the name >> > of the currently parsed input element in the visitor interface. For >> > top-level qdev properties, this is always set and matches 'prop->name'. >> > >> > However, for list elements it is NULL, because each element of a list >> > doesn't have a separate name. Passing a non-NULL value runs into >> > assertion failures in the visitor code. >> >> Yes. visitor.h's big comment: >> >> * The @name parameter of visit_type_FOO() describes the relation >> * between this QAPI value and its parent container. When visiting >> * the root of a tree, @name is ignored; when visiting a member of an >> * object, @name is the key associated with the value; when visiting a >> * member of a list, @name is NULL; and when visiting the member of an >> * alternate, @name should equal the name used for visiting the >> * alternate. >> >> > Therefore, using 'name' in error messages is not right for property >> > types that are used in lists, because "(null)" isn't very helpful to >> > identify what QEMU is complaining about. >> >> We get "(null)" on some hosts, and SEGV on others. > > Fair, I can add "(or even a segfault)" to the commit message. > >> Same problem in all property setters and getters (qdev and QOM), I >> presume. The @name parameter looks like a death trap. Thoughts? > > All property setters and getters that abuse @name instead of just > passing it to the visitor. I don't know how many do, but let's see: > > * qdev_prop_size32 uses @name in an error message > > * Array properties themselves use @name in an error message, too > (preexisting) > > * qdev-properties-system.c has more complicated properties, which have > the same problem: drive, chardev, macaddr, netdev (before this patch), > reserved_region, pci_devfn (one error path explicitly considers name > == NULL, but another doesn't), uuid > > * Outside of hw/core/ we may have some problematic properties, too. > I found qdev_prop_tpm, and am now too lazy to go through all devices. > These are probably specialised enough that they are less likely to be > used in arrays. > > We can fix these on top of this series, though we should probably first > figure out what the best way to replace it is to avoid touching > everything twice.
Makes sense. >> Any reproducer known before the next patch? > > Not to me. > >> > Change netdev properties to use 'prop->name' instead, which will contain >> > the name of the array property after switching array properties to lists >> > in the external interface. >> >> Points at the entire array property without telling the user which of >> the elements is at fault. Sure better than NULL. I'm not asking you to >> improve the error message further. Mention the error message's lack of >> precision in the commit message? > > Can do. > > At least the input visitor has better ways internally to identify the > currently visited object with full_name(). If other visitor types can do > similar things, maybe extending the public visitor interface to access > the name would be possible? Yes, there's a need for better error reporting around visitors, ideally without doing it in every visitor like the QObject input visitor does. I've thought about it, but only briefly, and without a conclusion. I figure your idea is to provide a visitor method to get a malloced string describing the thing being visited. Might work, but to know for sure, we have to try. This could also help with getting rid of the dangerous @name use we discussed above. Thanks!
