Re: [git pull] device mapper changes for 6.1

[Mikulas Patocka <mpatocka@redhat.com>]

On Tue, 18 Oct 2022, Linus Torvalds wrote:

> 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.

This is what we currently do.

>            Linus

The error string is not intended to be parsed by userspace (I agree that 
parsing the error string is a horrible idea, but this is not going to 
happen). It is intended to be displayed to the user by tools such as 
cryptsetup or integritysetup. The tool can't read the log, extract 
messages from it and display them.

With "just use errno", the user sees messages like "device-mapper: reload 
ioctl on test (254:0) failed: No such file or directory" and it's not much 
useful because it doesn't tell what went wrong.

Try to type "grep -r 'ti->error = ' drivers/md/|wc -l". There are 480 
distinct error messages generated by device mapper. You can't map each of 
them to a unique errno number.


BTW. we were talking about replacing device mapper version numbers with 
feature bitmaps and people preferred textual lists of features instead of 
bitmaps (because the bitmap will overflow when you have more than 64 
features). Do you oppose to this too? Do you prefer a 64-bit feature 
bitmap or a string with comma-separated list of features?

Mikulas