On Fri, Jun 07, 2024 at 05:26:53PM +0200, Christian Brauner wrote:
> On Fri, Jun 07, 2024 at 02:59:49AM +0100, Al Viro wrote:
> > low-hanging fruit; that's another likely source of conflicts
> > over the cycle and it might make a lot of sense to split;
> > fortunately, it can be split pretty much on per-function
> > basis - chunks are independent from each other.
> >
> > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > ---
>
> I can pick conversions from you for files where I already have changes
> in the tree anyway or already have done conversion as part of other
> patches.
Some notes:
This kind of conversions depends upon fdput(empty) being not just
a no-op, but a no-op visible to compiler. Representation change arranges
for that.
CLASS(...) has some misfeatures or nearly C++ level of tastelessness;
for this series I decided to try it and see how it goes, but... AFAICS in
a lot of cases it's the wrong answer.
1. Variable declarations belong in the beginning of block.
CLASS use invites violating that; to make things worse, in-block goto
bypassing such declaration is only caught by current clang. Two
examples got caught by Intel buildbot in this patch, actually - one
in fs/select.c, another in ocfs2.
2. The loss of control over the location of destructor call can
be dangerous in some cases. Delaying it is not a problem for struct
file references (well, except for goto problems above), but there's
an opposite issue. Example from later in this series:
if (arg != -1) {
struct perf_event *output_event;
struct fd output;
ret = perf_fget_light(arg, &output);
if (ret)
return ret;
output_event = output.file->private_data;
ret = perf_event_set_output(event, output_event);
fdput(output);
} else {
ret = perf_event_set_output(event, NULL);
}
gets converted to
if (arg != -1) {
struct perf_event *output_event;
CLASS(fd, output)(arg);
if (!is_perf_file(output))
return -EBADF;
output_event = fd_file(output)->private_data;
return perf_event_set_output(event, output_event);
} else {
return perf_event_set_output(event, NULL);
}
Nice, but that invites the next step, doesn't it? Like this:
struct perf_event *output_event = NULL;
if (arg != -1) {
CLASS(fd, output)(arg);
if (!is_perf_file(output))
return -EBADF;
output_event = fd_file(output)->private_data;
}
return perf_event_set_output(event, output_event);
See the trouble here? The last variant would be broken - the value of
file->private_data escapes the scope and can end up used after
the file gets closed.
In the original variant it's easy to see - we have an explicit
fdput() marking the place where protection disappears. After
the conversion to implicit cleanups we lose that.
Yes, use of __cleanup can nicely shrink the source and make
some leaks less likely. But my impression from this experiment
is that we should be very cautious with using that technics ;-/
