On Thu, 2 Mar 2023 09:52:31 -0500 Jes Sorensen <jes@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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
