From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xm0df-0007Fm-6u for qemu-devel@nongnu.org; Wed, 05 Nov 2014 08:28:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xm0da-0000lR-9u for qemu-devel@nongnu.org; Wed, 05 Nov 2014 08:28:43 -0500 References: <1415098223-32404-1-git-send-email-zhang.zhanghailiang@huawei.com> <1415098223-32404-2-git-send-email-zhang.zhanghailiang@huawei.com> <874mufumgk.fsf@linaro.org> <5459CC56.1030109@msgid.tls.msk.ru> <545A15C8.6090501@huawei.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <545A15C8.6090501@huawei.com> Date: Wed, 05 Nov 2014 13:28:27 +0000 Message-ID: <87zjc5srno.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH v3 1/5] qemu-char: fix parameter check in some qemu_chr_parse_* functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: zhanghailiang Cc: qemu-trivial@nongnu.org, Michael Tokarev , armbru@redhat.com, qemu-devel@nongnu.org, kraxel@redhat.com, peter.huangpeng@huawei.com zhanghailiang writes: > On 2014/11/5 15:05, Michael Tokarev wrote: >> 04.11.2014 16:25, Alex Bennée wrote: >>> zhanghailiang writes: >> a good reason for having assert here. >> > > Agreed, assert here is still not unnecessary, > filename will never to be NULL in these two cases. > >>> At a minimum I'd replace it with a g_assert(filename) to make the >>> calling contract clear. >> >> This is an internal set of APIs for a chr device, each kind is >> having a pair of functions which are called in order (first parse, >> next open), -- _that_ is the contract. assert isn't really about informing the user, it just makes an explicit statement that this API will always have a filled in filename and if it ever gets a NULL that's a programming bug in using the API, internal or otherwise. >> [] >>> All this boilerplate checking makes me think that either the qemu_opt >>> machinery should be ensuring we get a valid option string? >> >> Might be a good idea, yes, but that'd be a huge change, since that >> should be done in a lot of places, and in many cases we can't >> express our rules easily (eg, only one of two parameters should >> be present). I think at this stage adding simple checks to >> _parse functions is the way to go, and it is easy to read too. Yes, I wasn't intending to suggest expanding this patch set to encompase the larger task ;-) -- Alex Bennée