On 30/11/2024 19:51, Jonathan Cameron wrote:
On Thu, 28 Nov 2024 18:50:24 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
The error path in the gain_to_scaletables() uses goto for unwinding an
allocation on failure. This can be slightly simplified by using the
automated free when exiting the scope.
Use __free(kfree) and drop the goto based error handling.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
---
This is derived from the:
https://lore.kernel.org/lkml/5efc30d832275778d1f48d7e2c75b1ecc63511d5.1732105157.git.mazziesaccount@xxxxxxxxx/
Hi Matti
A few comments on specific parts of this below
Thanks,
Jonathan
+static int build_combined_table(struct iio_gts *gts, int **gains, size_t gain_bytes)
+{
+ int ret, i, j, new_idx, time_idx;
+ int *all_gains __free(kfree) = kcalloc(gts->num_itime, gain_bytes,
+ GFP_KERNEL);
+
if (!all_gains)
return -ENOMEM;
@@ -232,10 +238,9 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
gts->avail_all_scales_table = kcalloc(new_idx, 2 * sizeof(int),
GFP_KERNEL);
I'm not particularly keen in a partial application of __free magic.
I am starting to think the partial application of __free would actually
be what I preferred... (see below).
Perhaps you can use a local variable for this and a no_free_ptr() to assign it after we know
there can't be an error that requires it to be freed.
I am having second thoughts of this whole series. I do love the idea of
__free() magic, when applied on a simple temporary allocations that are
intended to be freed at the end of a function. Eg, for cases where we
know the scope from the very beginning. With a consistent use of __free
in such cases could make it much more obvious for a reader that this
stuff is valid only for a duration of this block. I have a feeling that
mixing the no_free_ptr() is a violation, and will obfuscate this.
I know I used it in this series while trying to simplify the flow - and
I am already regretting this.
Additionally, I indeed am not okay with introducing variables in middle
of a function. I do really feel quite strongly about that.
It seems that in many functions it makes sense to have some checks or
potentially failing operations done before doing memory allocations. So,
keeping the allocation at the start of a block can often require some
additional "check/do these things before calling an internal function
which does alloc + rest of the work" -wrappers.
It will then also mean that the internal function (called from a wrapper
with checks) will lack of the aforementioned checks, and, is thus
somehow unsafe. I am not saying such wrappers are always wrong -
sometimes it may be ok - but it probably should be consistent approach
and not a mixture of conventions depending on allocations...
Also, sometimes this would result some very strangely split functions
with no well defined purpose.
As a result, I would definitely only use the "__free magic" in places
where it does fit well for freeing a memory which is known to be needed
only for a specific block. And, I would only use it where the alloc can
be done at the beginning of a function in a rather natural way. This,
however, is very likely to lead in mixed use of "__free magic" and
regular allocs.
- if (!gts->avail_all_scales_table) {
- ret = -ENOMEM;
- goto free_out;
- }
+ if (!gts->avail_all_scales_table)
+ return -ENOMEM;
+
gts->num_avail_all_scales = new_idx;
for (i = 0; i < gts->num_avail_all_scales; i++) {
@@ -246,14 +251,25 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
if (ret) {
kfree(gts->avail_all_scales_table);
gts->num_avail_all_scales = 0;
- goto free_out;
+ return ret;
}
}
-free_out:
- kfree(all_gains);
+ return 0;
+}
- return ret;
+static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
+{
+ int ret;
+ size_t gain_bytes;
+
+ ret = fill_and_sort_scaletables(gts, gains, scales);
+ if (ret)
+ return ret;
+
+ gain_bytes = array_size(gts->num_hwgain, sizeof(int));
array_size is documented as being for 2D arrays, not an array of a multi byte
type. We should not use it for this purpose.
Thanks for pointing this out. I was not familiar with that. I am
actually pretty sure that using the array_size() has been recommended to
me :)
I'd be tempted to not worry about overflow, but if you do want to be sure then
copy what kcalloc does and use a check_mul_overflow()
https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/slab.h#L919
Thanks for the direct pointer :) I'll take a look at it!
You don't have to tidy that up in this patch though.
+
+ return build_combined_table(gts, gains, gain_bytes);
}
+/**
+ * iio_gts_build_avail_time_table - build table of available integration times
+ * @gts: Gain time scale descriptor
+ *
+ * Build the table which can represent the available times to be returned
+ * to users using the read_avail-callback.
+ *
+ * NOTE: Space allocated for the tables must be freed using
+ * iio_gts_purge_avail_time_table() when the tables are no longer needed.
+ *
+ * Return: 0 on success.
+ */
+static int iio_gts_build_avail_time_table(struct iio_gts *gts)
Hmm. I guess this wrapper exists because perhaps you aren't comfortable
yet with the __free() handling mid function. It does not harm so I'm fine
having this.
Yes. This was the reason for this wrapper. But I'm not really happy
about the wrappers either (even if I agree with you that it does not
really hurt here). Furthermore, if you're feeling strongly about not
mixing the __free and regular allocs, then I simply prefer not using the
magic one. I don't think using the __free with allocations intended to
last longer than the scope is clear. Ues, goto sequences may not always
be simple, but at least people are used to be wary with them.
+{
+ if (!gts->num_itime)
+ return 0;
+
+ return __iio_gts_build_avail_time_table(gts);
+}
+
/**
* iio_gts_purge_avail_time_table - free-up the available integration time table
* @gts: Gain time scale descriptor
Thanks a ton for the review :) It helped me to clarify my thoughts on this!
Yours,
-- Matti
