Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Thu, 2 Mar 2023 09:52:31 -0500
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 12/29/22 04:39, Mariusz Tkaczyk wrote:
> 
> Hi Mariusz,
> 
> Apologies for the slow response on this one.
> 
> > On Wed, 28 Dec 2022 10:07:22 -0500
> > Jes Sorensen <jes@trained-monkey.org> wrote:  
> 
> >> I appreciate the work to consolidate duplicate code. However, I am not a
> >> fan of new typedefs, in addition you return status_t codes in functions
> >> changed to return error_t, which is inconsistent.  
> > 
> > Hi Jes,
> > Indeed, initially I named it as error_t and I forgot to update that part.
> > I'm surprised that compiler didn't catch it. Thanks!
> > 
> > About typedef, I did it same for IMSM already:
> > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/super-intel.c#n376
> > I can change that but I wanted to define a common solution propagated later
> > to other mdadm parts.  
> 
> I am really on the fence on this one. I'd very much like to see us get
> away from the nasty 0/1/2 error codes we currently have all over the
> place, but I am also vary of reinventing the wheel.
> 
> I must admit I missed it in super-intel.c
> 
> >> I would prefer if we move towards standard POSIX error codes instead of
> >> trying to invent new ones.
> >>  
> > 
> > The POSIX errors are defined for communication with kernel space and
> > unfortunately they are not detailed enough. For example "undefined" or
> > just "general_error" statuses are not available.
> > https://man7.org/linux/man-pages/man3/errno.3.html
> > It the approach I proposed we are free to create exact errors we need.
> > Later we can create a map of error values to string and create dedicated
> > error print functions.  
> 
> I agree that POSIX codes aren't perfect, however at least for the
> current errors I see reported in this patch -EINVAL or -E2BIG ought to
> cover. If you think there are many cases where we cannot map well to
> POSIX, then I would be OK with it, but I would prefer to go straight to
> a global error code space rather than one per subsystem.
> 
> Thoughts?
> 
Hi Jes,

I was in this place with ledmon project:
https://github.com/intel/ledmon/blob/master/src/status.h

Yeah, it seems to be overhead to maintain error enum per each subsystem yet, you
are right here. I don't plan huge code reactor to make those enums common used.
If we don't plan mdadm library, then there is no need to handle define multiple
enums. It has real sense if we want to hide some statuses from library user
inside our code.

I would like to handle internal error definitions because I think that mdadm
deserves flexibility to define status what it needs. In general case we needs
just *error* but when it comes to more advanced flows I can see multiple
meaningful statuses we need to differentiate. The goal here is to make errors
straightforward.

I don't consider it as reinventing the wheel because the software is free
to define error handlers as it wants. There are no restrictions and POSIX codes
are not a solution. POSIX codes are available to us because we need to
understand kernel error codes and we need to handle them and react
appropriately.

Simply, I would like to have error possibly the best self described and the
error enum allows me to achieve that. It also forces developers to
follow the statuses we have there because if something like that is defined,
there is high probability that maintainer will ask developer to use this enum
in new function and use meaningful error codes respectively.

I would like to add this enum to mdadm.h but I will avoid to adding more enum
in the future if there is no need to. Let me know if that works for you.

Thanks for discussion!
Mariusz