Markus Armbruster <[email protected]> writes: > Fabiano Rosas <[email protected]> writes: > >> It's a common pattern in vl.c to chain the qemu_find_opts() output >> into the first argument of qemu_opts_parse_noisily(): >> >> opts = qemu_opts_parse_noisily(qemu_find_opts("spice"), optarg, false); >> if (!opts) { >> exit(1); >> } >> >> In cases such as spice that have the group defined in the module file, >> it's possible to reach qemu_opts_parse_noisily() with a NULL QemuOptsList if >> the module is not present in the host filesystem. >> >> $ ../configure --enable-modules --enable-spice >> $ make >> $ mv qemu-bundle/usr/local/lib64/qemu/ui-spice-core.so{,.not} >> $ ./qemu-system-x86_64 -spice a >> qemu-system-x86_64: -spice a: There is no option group 'spice' >> Segmentation fault (core dumped) >> >> Return NULL from qemu_opts_parse_noisily() if there is no list. >> >> Signed-off-by: Fabiano Rosas <[email protected]> >> --- >> util/qemu-option.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/util/qemu-option.c b/util/qemu-option.c >> index 9fbf425f86..0ec12d252c 100644 >> --- a/util/qemu-option.c >> +++ b/util/qemu-option.c >> @@ -945,6 +945,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, >> const char *params, >> QemuOpts *opts; >> bool help_wanted = false; >> >> + if (!list) { >> + return NULL; >> + } >> + >> opts = opts_parse(list, params, permit_abbrev, true, >> opts_accepts_any(list) ? NULL : &help_wanted, >> &err); > > Before the patch, qemu_opts_parse_noisily() either > > * Succeeds and returns non-null > > * Fails, reports an error, and returns null > > * Prints help and returns null > > Your patch adds a fourth case: > > * Fails silently and returns null > > I dislike this case. Functions should either always print something > whent they fail, or never. > > Are all callers prepared for silent failure? > > The pattern you quoted in the commit message is, because > qemu_find_opts() reports an error. > > Here's a cleaner solution for this pattern. Replace > > opts = qemu_opts_parse_noisily(qemu_find_opts(...), ...) > > by a call of a new helper function that does > > list = qemu_find_opts(...); > if (!list) { > return NULL; > } > return qemu_opts_parse_noisily(list, ...); > > Thoughts?
I agree, but I didn't want to add another helper on top of qemu_opts_parse_noisily() so I did some further cleanup, see whether you hate it: https://lore.kernel.org/r/[email protected]
