From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5FB14E7719C for ; Fri, 10 Jan 2025 13:33:51 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1tWF8N-0001wj-BQ; Fri, 10 Jan 2025 08:33:07 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tWF8M-0001wa-0T for qemu-devel@nongnu.org; Fri, 10 Jan 2025 08:33:06 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tWF8K-000747-2P for qemu-devel@nongnu.org; Fri, 10 Jan 2025 08:33:05 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736515981; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=elbS5kxOz81zaT/VZFcGTo4rbCsnR13yroBGsDtO+zM=; b=eCCPWZ4FImyVcjhhkC+Xxyap5NQXveWKAMQlC3K93PNr2B4pTFXLyltO0BkahOvGQXSoKU /NYOqJgz7uTMmcoaBkZBMhvKtyUq3XvgGOjRBcXNfOE/jJKPLHOkIMpoQN9++boWSKgG+M qlP3vg6vj1XLslhRwfg3/+Xw52Og+fg= Received: from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-488-QjGulpivN32U-FVcFet7cA-1; Fri, 10 Jan 2025 08:32:58 -0500 X-MC-Unique: QjGulpivN32U-FVcFet7cA-1 X-Mimecast-MFC-AGG-ID: QjGulpivN32U-FVcFet7cA Received: from mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.17]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 9EE7D1956050; Fri, 10 Jan 2025 13:32:55 +0000 (UTC) Received: from redhat.com (unknown [10.42.28.82]) by mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 114031955BE3; Fri, 10 Jan 2025 13:32:51 +0000 (UTC) Date: Fri, 10 Jan 2025 13:32:48 +0000 From: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= To: Ilya Leoshkevich Cc: Warner Losh , Riku Voipio , Laurent Vivier , Paolo Bonzini , Richard Henderson , Alex =?utf-8?Q?Benn=C3=A9e?= , Kyle Evans , Philippe =?utf-8?Q?Mathieu-Daud=C3=A9?= , qemu-devel@nongnu.org Subject: Re: [PATCH v4 1/9] qapi: Make qapi_bool_parse() gracefully handle NULL value Message-ID: References: <20250108202625.149869-1-iii@linux.ibm.com> <20250108202625.149869-2-iii@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/2.2.13 (2024-03-09) X-Scanned-By: MIMEDefang 3.0 on 10.30.177.17 Received-SPF: pass client-ip=170.10.133.124; envelope-from=berrange@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -24 X-Spam_score: -2.5 X-Spam_bar: -- X-Spam_report: (-2.5 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.432, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org On Fri, Jan 10, 2025 at 02:03:09PM +0100, Ilya Leoshkevich wrote: > On Fri, 2025-01-10 at 11:33 +0000, Daniel P. Berrangé wrote: > > On Wed, Jan 08, 2025 at 09:04:56PM +0100, Ilya Leoshkevich wrote: > > > Use g_strcmp0(), so that NULL is considered an invalid parameter > > > value. > > > > Why are we calling qapi_bool_parse with a NULL value in the first > > place ? IMHO this is a sign of a bug higher up the call chain > > that ought to be fixed, as in general all our input parsing code > > would expect non-NULL input values. > > The intended use case is the following (patch 7/9): > > g_auto(GStrv) tokens = g_strsplit(*arg, "=", 2); > Error *err = NULL; > > if (g_strcmp0(tokens[0], "suspend") == 0) { > if (!qapi_bool_parse(tokens[0], tokens[1], &suspend, &err)) { > warn_report_err(err); > [...] > > The idea is to uniformly handle "suspend=y", "suspend=invalid" and > "suspend"; the latter requires checking whether token[1] is NULL. > Of course, this can be special-cased in the caller, but this would be > less elegant. On the contrary, I think handling the latter in the caller explicitly is the correct approach. As written this code snippet gives readers the misleading impression that 'tokens[1]' is expected to be a non-NULL bool value. It is not at all obvious that the code is intentionally trying to support a NULL value for tokens[1]. Even if we can see that qapi_bool_parse accepts NULL, a reader has no idea whether this caller has intentionally passed NULL or simply forgot to raise an error when NULL is seen. IMHO this patch should be dropped, and the desired behaviour made explicit in patch 7. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|