Received-SPF: pass (google.com: domain of linux-kernel+bounces-111448-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <8a1adff2-eb83-4dec-b8d0-1e523245de65@web.de>
Date: Fri, 22 Mar 2024 14:00:42 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Dan Williams <dan.j.williams@intel.com>,
 Peter Zijlstra <peterz@infradead.org>,
 Linus Torvalds <torvalds@linux-foundation.org>,
 Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
 kernel-janitors@vger.kernel.org
Cc: Bjorn Helgaas <bhelgaas@google.com>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
 Ira Weiny <ira.weiny@intel.com>,
 Jesse Brandeburg <jesse.brandeburg@intel.com>,
 Jonathan Cameron <Jonathan.Cameron@huawei.com>,
 Julia Lawall <Julia.Lawall@inria.fr>, Lukas Wunner <lukas.wunner@intel.com>
References: <171097196970.1011049.9726486429680041876.stgit@dwillia2-xfh.jf.intel.com>
Subject: Re: [PATCH] cleanup: Add usage and style documentation
Content-Language: en-GB
From: Markus Elfring <Markus.Elfring@web.de>
In-Reply-To: <171097196970.1011049.9726486429680041876.stgit@dwillia2-xfh.jf.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:sDd5Oswr9wg=;oUpHLGsbXWg5ca5bC8NBGfysCrf
 DFv6hUPq8S4H/XwQpwPiQuK4HLa3C2XsqohEPgF8DqzZ2fDNQxfq5ZjtnTiRXOh+cIfSx0si5
 I8p6NBAwC4kK18ez+vuj4QBiYZaedbKGinfljFLez8lz3PFDPoXvsJZ9FhWQUIY8x0KLDejZj
 9SYVMpL7Hy2KSi85kGlwTXLj+OwNqHVdoBcZHVwXjiXUQLCFI9WqvY3zmPxKgYC3x1ql/1DeO
 6iXHJ4zA7rzyBFgnZwFhA+sloNmWRsp7UQr5yT9HUFot1v3W//MS9grpfZoKs17jL7RM+7mqq
 VTJczbJTQY5/TS/miewAlPcLmi2qur+NZtXsdGMrOLgkpNuuwpVW5CylSlyQHwxHAl5OQSC0n
 hE/yVcRxZ+iIH1RMl6m9TNwEBvd1uvg4XvMbDaxPCLLbSQjp62otI96yLupwQNyHkvyOFlH6m
 0kCUa+/bWDP8a8C72EV5qelNJEE3zux7FKyL6bWbZVjo6zs8T3HP0h9fmKjtnBSms3Jp6Lpui
 QmlqSE+Rae1sJUDalnx1KDcXNGI+N1D/aGvmytshJ1PgS4VRXj/hwjyrINOLgkGHuD7LuSscy
 jDoL363DGW2A1q5YWiEPzqKg0W7zBMepvkloBNElY5oIHN/GZNqfAVf5dhTyXqhWtbLLpbnHc
 4f98L+PSD4h17gPOZusT1LB3gQCsD+MYLkKzIOfkNOTMpkpqoIccy7Xu8ao+FYHduF5742Ut/
 roZZ91Y7fK8Bk2rKmukct+SBsyqR+6lB8+CB5nRb7ybqmJXj0S33GNNL8NMNtwh15WRT74dZ9
 ak5aJWDVHTUL0WWtPbmSRaM3uFSuKvoFJUegBWNPntwQk=

=E2=80=A6
> +++ b/include/linux/cleanup.h
> @@ -4,6 +4,118 @@
>
>  #include <linux/compiler.h>
>
> +/**
> + * DOC: scope-based cleanup helpers
> + *
> + * The "goto error" pattern is notorious for introducing =E2=80=A6

Will any other label become more helpful for this description approach?


> + * this tedium and can aid in maintaining FILO (first in last out)
             =E2=AC=86
Would an other word be more appropriate here?


> + * contraindicates a pattern like the following:

I would prefer an other wording approach.


> + *	struct pci_dev *dev __free(pci_dev_put) =3D NULL;

Programmers got used to null pointer initialisations.


> + * In this case @dev is declared in x-mas tree style in a preamble
> + * declaration block. That is problematic because it destroys the
> + * compiler's ability to infer proper unwind order.

Can capabilities be clarified better for the applied compilers?


>                                                      If other cleanup
> + * helpers appeared in such a function that depended on @dev being live
> + * to complete their unwind then using the "struct obj_type *obj
> + * __free(...) =3D NULL" style is an anti-pattern that potentially caus=
es
> + * a use-after-free bug.

I suggest to reconsider such a development concern in more detail.


> + *	struct pci_dev *dev __free(pci_dev_put) =3D
> + *		pci_get_slot(parent, PCI_DEVFN(0, 0));
> + *
> + * ...which implies that declaring variables in mid-function scope is
> + * not only allowed, but expected.

* Is there a need to separate the ellipsis from the subsequent word
  by a space character?

* You propose a variable definition without specifying extra curly bracket=
s
  (for another compound statement / code block).
  This can work only if an appropriate pointer is returned by the called f=
unction.

* The involved identifiers can occasionally get longer.
  Further code layout challenges would need corresponding clarifications.
  How will the handling of line length concerns evolve?

* I suggest to take another look also at the transformation pattern
  =E2=80=9CReduce Scope of Variable=E2=80=9D.
  https://refactoring.com/catalog/reduceScopeOfVariable.html


> + * Conversions of existing code to use cleanup helpers should convert
> + * all resources so that no "goto" unwind statements remain. If not all
> + * resources are amenable to cleanup then additional refactoring is
> + * needed to build helper functions, or the function is simply not a
> + * good candidate for conversion.

* How do you think about to specify any more resource cleanup functions
  for growing usage of =E2=80=9Csmart pointers=E2=80=9D?

* Would you like to extend the specification of function pairs for
  improved applications of guard variants?


Regards,
Markus