From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56346) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZvFg-0006i7-Py for qemu-devel@nongnu.org; Wed, 17 Aug 2016 03:27:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bZvFd-0002jw-Ha for qemu-devel@nongnu.org; Wed, 17 Aug 2016 03:27:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34746) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZvFd-0002js-Bo for qemu-devel@nongnu.org; Wed, 17 Aug 2016 03:27:01 -0400 From: Markus Armbruster References: <50710af43689d251448f6b2f8d5606956758c998.1471360024.git.mprivozn@redhat.com> Date: Wed, 17 Aug 2016 09:26:58 +0200 In-Reply-To: (Peter Maydell's message of "Tue, 16 Aug 2016 17:25:49 +0100") Message-ID: <874m6jvosd.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] qemu_opt_foreach: Fix crasher List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Michal Privoznik , "Michael S. Tsirkin" , QEMU Developers Peter Maydell writes: > On 16 August 2016 at 16:17, Michal Privoznik wrote: >> The solution is to teach qemu_opt_foreach() to take a shortcut if >> @opts is NULL. Please provide a reproducer. A stack backtrace wouldn't hurt. >> >> Signed-off-by: Michal Privoznik >> --- >> >> Even after this patch I'm still unable to attach vhost-user: >> >> {"id": "libvirt-20", "error": {"class": "GenericError", "desc": "chardev \"charnet2\" is not a unix socket"}} >> >> But at least, qemu does not crash anymore. >> >> util/qemu-option.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/util/qemu-option.c b/util/qemu-option.c >> index 3467dc2..78be7e1 100644 >> --- a/util/qemu-option.c >> +++ b/util/qemu-option.c >> @@ -614,6 +614,11 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, >> QemuOpt *opt; >> int rc; >> >> + if (!opts) { >> + /* Done, trivially. */ >> + return 0; >> + } >> + >> QTAILQ_FOREACH(opt, &opts->head, next) { >> rc = func(opaque, opt->name, opt->str, errp); >> if (rc) { >> -- >> 2.8.4 > > This seems plausible, but I don't understand our option > code very well, and we seem to have a mix of "check for > NULL" and "caller had better not pass NULL" in the various > functions in util/qemu-option.c. > > Markus: how is this supposed to work? I wouldn't say this is "supposed to work" in some specific way. "Happens to work" would be closer to the truth. If you want me to interpret some sense into the mess after the fact, here's my best guess: we generally require non-null opts, except for qemu_opts_del() and the qemu_opt_get_FOO(). Makes obvious sense for qemu_opts_del(), since when a failing constructor returns null, the destructor should accept null. The qemu_opt_get_FOO() feel like a (possibly misguided) attempt at convenience to me. > In any case something is clearly still busted in the > vhost-user code, because it's expecting to get a non-NULL > opts so it can properly parse the chardev, so that seems > like the thing we really need to fix. Probably. If I had a reproducer or at least a stack backtrace, I'd even know where to look.