Re: [git pull] device mapper changes for 6.1

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: Mike Snitzer <snitzer@kernel.org>,
	dm-devel@redhat.com, linux-block@vger.kernel.org,
	Alasdair G Kergon <agk@redhat.com>,
	Genjian Zhang <zhanggenjian@kylinos.cn>,
	Jiangshan Yi <yijiangshan@kylinos.cn>,
	Jilin Yuan <yuanjilin@cdjrlc.com>,
	Mikulas Patocka <mpatocka@redhat.com>,
	Milan Broz <gmazyland@gmail.com>,
	Nathan Huckleberry <nhuck@google.com>,
	Nikos Tsironis <ntsironis@arrikto.com>,
	Shaomin Deng <dengshaomin@cdjrlc.com>
Subject: Re: [git pull] device mapper changes for 6.1
Date: Tue, 18 Oct 2022 11:54:49 -0700	[thread overview]
Message-ID: <CAHk-=wg3cpPyoO8u+8Fu1uk86bgTUYanfKhmxMsZzvY_mV=Cxw@mail.gmail.com> (raw)
In-Reply-To: <Y07twbDIVgEnPsFn@infradead.org>

On Tue, Oct 18, 2022 at 11:17 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Oct 18, 2022 at 12:20:50PM -0400, Mike Snitzer wrote:
> >
> > - Enhance DM ioctl interface to allow returning an error string to
> >   userspace. Depends on exporting is_vmalloc_or_module_addr() to allow
> >   DM core to conditionally free memory allocated with kasprintf().
>
> That really does not sound like a good idea at all.  And it does not
> seem to have any MM or core maintainer signoffs.

I wouldn't worry about maintainer sign-offs just for exporting a
helper function, but I agree with the whole concept being a complete
disaster and not a good idea at all.

Use errno.

It really is that simple. Strings have been discussed before, and they
are simply not a good idea. If your interface is so complicated that
you think errors need some textual explanation, your interface is
probably garbage.

Strings also have allocation issues (as you found out), and have
serious localization issues.

Yes, we do a lot of strings in the kernel in the form of dmesg, and we
have the rule that we simply don't localize. But that's dmesg. It's
for special stuff, not some interface.

And equally importantly, some really small detail in the kernel really
has *NO* business making up new error models of its own. You may think
that the DM ioctl's are a big and important deal, but realistically,
it's just an odd corner of the world that very very few people care
about, and they can use the same error numbers that EVERYBODY ELSE HAS
BEEN USING FOR SIX DECADES!

Don't reinvent something that works - badly.

I think we have one major interface that is string-based (apart from
the obvious pathname ones and the strings passed to 'execve()').

It's 'mount()' (and now fsconfig() etc), and it's string-based mainly
because it has that nasty "arbitrary things that different filesystem
may need for configuration"). And it has some nasty logging model
associated with it too for output.

But no, we absolutely do *not* want to emulate that particular horror
anywhere else.

If you think some errors are really important and hard to understand,
maybe you can just log them with a ratelimited pr_info() or something.

           Linus