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

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

On Wed, 28 Dec 2022 10:07:22 -0500
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 12/21/22 06:50, Mariusz Tkaczyk wrote:
> > Move duplicated code for both config.c and mdadm.c to new functions.
> > Add error enum in mdadm.h. Use MD_NAME_MAX instead hardcoded value
> > in mddev_ident. Use secure functions.
> > 
> > In next patch POSIX validation is added.
> > 
> > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>  
> 
> Hi Mariusz,
> 
> 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 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.

Thanks
Mariusz

> 
> > ---
> >  config.c | 35 +++++++++++++++++++++++++++--------
> >  lib.c    | 16 ++++++++++++++++
> >  mdadm.c  | 16 ++++------------
> >  mdadm.h  | 21 +++++++++++++++------
> >  util.c   | 20 ++++++++++++++++++++
> >  5 files changed, 82 insertions(+), 26 deletions(-)
> > 
> > diff --git a/config.c b/config.c
> > index eeedd0c6..7d3eb6fc 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -147,6 +147,32 @@ inline void ident_init(struct mddev_ident *ident)
> >  	ident->uuid_set = 0;
> >  }
> >  
> > +/**
> > + * ident_set_name()- set name in &mddev_ident.
> > + * @ident: pointer to &mddev_ident.
> > + * @name: name to be set.
> > + *
> > + * If criteria passed, set name in @ident.
> > + *
> > + * Return: %STATUS_SUCCESS or %STATUS_ERROR.
> > + */
> > +status_t ident_set_name(struct mddev_ident *ident, const char *name)
> > +{
> > +	assert(name);
> > +	assert(ident);
> > +
> > +	if (ident->name[0]) {
> > +		pr_err("Name can be specified once, second value is
> > '%s'.\n", name);
> > +		return STATUS_ERROR;
> > +	}
> > +
> > +	if (check_mdname(name) == STATUS_ERROR)
> > +		return STATUS_ERROR;
> > +
> > +	snprintf(ident->name, MD_NAME_MAX + 1, "%s", name);
> > +	return STATUS_SUCCESS;
> > +}
> > +
> >  struct conf_dev {
> >  	struct conf_dev *next;
> >  	char *name;
> > @@ -445,14 +471,7 @@ void arrayline(char *line)
> >  					mis.super_minor = minor;
> >  			}
> >  		} else if (strncasecmp(w, "name=", 5) == 0) {
> > -			if (mis.name[0])
> > -				pr_err("only specify name once, %s
> > ignored.\n",
> > -					w);
> > -			else if (strlen(w + 5) > 32)
> > -				pr_err("name too long, ignoring %s\n", w);
> > -			else
> > -				strcpy(mis.name, w + 5);
> > -
> > +			ident_set_name(&mis, w + 5);
> >  		} else if (strncasecmp(w, "bitmap=", 7) == 0) {
> >  			if (mis.bitmap_file)
> >  				pr_err("only specify bitmap file once. %s
> > ignored\n", diff --git a/lib.c b/lib.c
> > index e395b28d..624580e1 100644
> > --- a/lib.c
> > +++ b/lib.c
> > @@ -27,6 +27,22 @@
> >  #include	<ctype.h>
> >  #include	<limits.h>
> >  
> > +/**
> > + * is_strnlen_lq() - Check if string length is lower or equal to requested.
> > + * @str: string to check.
> > + * @max_len: max length.
> > + *
> > + * @str length must be bigger than 0 and be lower or equal @max_len,
> > including termination byte.
> > + */
> > +bool is_strnlen_lq(const char * const str, size_t max_len)
> > +{
> > +	assert(str);
> > +
> > +	size_t _len = strnlen(str, max_len);
> > +
> > +	return (_len < max_len && _len != 0);
> > +}
> > +
> >  bool is_dev_alive(char *path)
> >  {
> >  	if (!path)
> > diff --git a/mdadm.c b/mdadm.c
> > index 74fdec31..5bd3d5a7 100644
> > --- a/mdadm.c
> > +++ b/mdadm.c
> > @@ -686,20 +686,14 @@ int main(int argc, char *argv[])
> >  		case O(CREATE,'N'):
> >  		case O(ASSEMBLE,'N'):
> >  		case O(MISC,'N'):
> > -			if (ident.name[0]) {
> > -				pr_err("name cannot be set twice.   Second
> > value %s.\n", optarg);
> > -				exit(2);
> > -			}
> >  			if (mode == MISC && !c.subarray) {
> >  				pr_err("-N/--name only valid with
> > --update-subarray in misc mode\n"); exit(2);
> >  			}
> > -			if (strlen(optarg) > 32) {
> > -				pr_err("name '%s' is too long, 32 chars
> > max.\n",
> > -					optarg);
> > +
> > +			if (ident_set_name(&ident, optarg) !=
> > STATUS_SUCCESS) exit(2);
> > -			}
> > -			strcpy(ident.name, optarg);
> > +
> >  			continue;
> >  
> >  		case O(ASSEMBLE,'m'): /* super-minor for array */
> > @@ -1341,10 +1335,8 @@ int main(int argc, char *argv[])
> >  		} else {
> >  			char *bname = basename(devlist->devname);
> >  
> > -			if (strlen(bname) > MD_NAME_MAX) {
> > -				pr_err("Name %s is too long.\n",
> > devlist->devname);
> > +			if (check_mdname(bname))
> >  				exit(1);
> > -			}
> >  
> >  			ret = stat(devlist->devname, &stb);
> >  			if (ident.super_minor == -2 && ret != 0) {
> > diff --git a/mdadm.h b/mdadm.h
> > index 23ffe977..b68fa4bc 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -275,6 +275,11 @@ static inline void __put_unaligned32(__u32 val, void
> > *p) 
> >  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
> >  
> > +/**
> > + * This is true for native and DDF, IMSM allows 16.
> > + */
> > +#define MD_NAME_MAX 32
> > +
> >  extern const char Name[];
> >  
> >  struct md_bb_entry {
> > @@ -406,6 +411,12 @@ struct spare_criteria {
> >  	unsigned int sector_size;
> >  };
> >  
> > +typedef enum status {
> > +	STATUS_SUCCESS = 0,
> > +	STATUS_ERROR,
> > +	STATUS_UNDEF,
> > +} status_t;
> > +
> >  enum mode {
> >  	ASSEMBLE=1,
> >  	BUILD,
> > @@ -528,7 +539,7 @@ struct mddev_ident {
> >  
> >  	int	uuid_set;
> >  	int	uuid[4];
> > -	char	name[33];
> > +	char	name[MD_NAME_MAX + 1];
> >  
> >  	int super_minor;
> >  
> > @@ -1538,6 +1549,7 @@ extern int check_partitions(int fd, char *dname,
> >  extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
> >  extern int stat_is_blkdev(char *devname, dev_t *rdev);
> >  
> > +extern bool is_strnlen_lq(const char * const str, size_t max_len);
> >  extern bool is_dev_alive(char *path);
> >  extern int get_mdp_major(void);
> >  extern int get_maj_min(char *dev, int *major, int *minor);
> > @@ -1555,6 +1567,7 @@ extern void manage_fork_fds(int close_all);
> >  extern int continue_via_systemd(char *devnm, char *service_name);
> >  
> >  extern void ident_init(struct mddev_ident *ident);
> > +extern status_t ident_set_name(struct mddev_ident *ident, const char
> > *name); 
> >  extern int parse_auto(char *str, char *msg, int config);
> >  extern struct mddev_ident *conf_get_ident(char *dev);
> > @@ -1634,6 +1647,7 @@ extern void print_r10_layout(int layout);
> >  
> >  extern char *find_free_devnm(int use_partitions);
> >  
> > +extern error_t check_mdname(const char *name);
> >  extern void put_md_name(char *name);
> >  extern char *devid2kname(dev_t devid);
> >  extern char *devid2devnm(dev_t devid);
> > @@ -1923,11 +1937,6 @@ enum r0layout {
> >  /* And another special number needed for --data_offset=variable */
> >  #define VARIABLE_OFFSET 3
> >  
> > -/**
> > - * This is true for native and DDF, IMSM allows 16.
> > - */
> > -#define MD_NAME_MAX 32
> > -
> >  /**
> >   * is_container() - check if @level is &LEVEL_CONTAINER
> >   * @level: level v> 
> > diff --git a/config.c b/config.c
> > index dc1620c1..eeedd0c6 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -119,6 +119,34 @@ int match_keyword(char *word)
> >  	return -1;
> >  }
> >  
> > +/**
> > + * ident_init() - Set defaults.
> > + * @ident: ident pointer, not NULL.
> > + */
> > +inline void ident_init(struct mddev_ident *ident)
> > +{
> > +	assert(ident);
> > +
> > +	ident->assembled = false;
> > +	ident->autof = 0;
> > +	ident->bitmap_fd = -1;
> > +	ident->bitmap_file = NULL;
> > +	ident->container = NULL;
> > +	ident->devices = NULL;
> > +	ident->devname = NULL;
> > +	ident->level = UnSet;
> > +	ident->member = NULL;
> > +	ident->name[0] = 0;
> > +	ident->next = NULL;
> > +	ident->raid_disks = UnSet;
> > +	ident->spare_group = NULL;
> > +	ident->spare_disks = 0;
> > +	ident->st = NULL;
> > +	ident->super_minor = UnSet;
> > +	ident->uuid[0] = 0;
> > +	ident->uuid_set = 0;
> > +}
> > +
> >  struct conf_dev {
> >  	struct conf_dev *next;
> >  	char *name;
> > @@ -363,22 +391,7 @@ void arrayline(char *line)
> >  	struct mddev_ident mis;
> >  	struct mddev_ident *mi;
> >  
> > -	mis.uuid_set = 0;
> > -	mis.super_minor = UnSet;
> > -	mis.level = UnSet;
> > -	mis.raid_disks = UnSet;
> > -	mis.spare_disks = 0;
> > -	mis.devices = NULL;
> > -	mis.devname = NULL;
> > -	mis.spare_group = NULL;
> > -	mis.autof = 0;
> > -	mis.next = NULL;
> > -	mis.st = NULL;
> > -	mis.bitmap_fd = -1;
> > -	mis.bitmap_file = NULL;
> > -	mis.name[0] = 0;
> > -	mis.container = NULL;
> > -	mis.member = NULL;
> > +	ident_init(&mis);
> >  
> >  	for (w = dl_next(line); w != line; w = dl_next(w)) {
> >  		if (w[0] == '/' || strchr(w, '=') == NULL) {
> > diff --git a/mdadm.c b/mdadm.c
> > index 972adb52..74fdec31 100644
> > --- a/mdadm.c
> > +++ b/mdadm.c
> > @@ -107,25 +107,13 @@ int main(int argc, char *argv[])
> >  
> >  	srandom(time(0) ^ getpid());
> >  
> > -	ident.uuid_set = 0;
> > -	ident.level = UnSet;
> > -	ident.raid_disks = UnSet;
> > -	ident.super_minor = UnSet;
> > -	ident.devices = 0;
> > -	ident.spare_group = NULL;
> > -	ident.autof = 0;
> > -	ident.st = NULL;
> > -	ident.bitmap_fd = -1;
> > -	ident.bitmap_file = NULL;
> > -	ident.name[0] = 0;
> > -	ident.container = NULL;
> > -	ident.member = NULL;
> > -
> >  	if (get_linux_version() < 2006015) {
> >  		pr_err("This version of mdadm does not support kernels
> > older than 2.6.15\n"); exit(1);
> >  	}
> >  
> > +	ident_init(&ident);
> > +
> >  	while ((option_index = -1),
> >  	       (opt = getopt_long(argc, argv, shortopt, long_options,
> >  				  &option_index)) != -1) {
> > diff --git a/mdadm.h b/mdadm.h
> > index 3673494e..23ffe977 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -33,8 +33,10 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t
> > __offset, int __whence)); # endif
> >  #endif
> >  
> > +#include	<assert.h>
> >  #include	<sys/types.h>
> >  #include	<sys/stat.h>
> > +#include	<stdarg.h>
> >  #include	<stdint.h>
> >  #include	<stdlib.h>
> >  #include	<time.h>
> > @@ -1552,6 +1554,8 @@ extern void enable_fds(int devices);
> >  extern void manage_fork_fds(int close_all);
> >  extern int continue_via_systemd(char *devnm, char *service_name);
> >  
> > +extern void ident_init(struct mddev_ident *ident);
> > +
> >  extern int parse_auto(char *str, char *msg, int config);
> >  extern struct mddev_ident *conf_get_ident(char *dev);
> >  extern struct mddev_dev *conf_get_devs(void);
> > @@ -1779,8 +1783,7 @@ static inline sighandler_t signal_s(int sig,
> > sighandler_t handler) #define dprintf_cont(fmt, arg...) \
> >          ({ if (0) fprintf(stderr, fmt, ##arg); 0; })
> >  #endif
> > -#include <assert.h>
> > -#include <stdarg.h>
> > +
> >  static inline int xasprintf(char **strp, const char *fmt, ...) {
> >  	va_list ap;
> >  	int ret;alue
> > diff --git a/util.c b/util.c
> > index 26ffdcea..b2a4ea21 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -932,6 +932,26 @@ int get_data_disks(int level, int layout, int
> > raid_disks) return data_disks;
> >  }
> >  
> > +/**
> > + * check_md_name()- check if MD device name is correct.
> > + * @name: name to check.
> > + *
> > + * In case of error, message is printed.
> > + *
> > + * Return: %STATUS_SUCCESS or %STATUS_ERROR.
> > + */
> > +error_t check_mdname(const char * const name)
> > +{
> > +	assert(name);
> > +
> > +	if (!is_strnlen_lq(name, MD_NAME_MAX + 1)) {
> > +		pr_err("Name '%s' is too long or empty, %d characters
> > max.\n", name, MD_NAME_MAX);
> > +		return STATUS_ERROR;
> > +	}
> > +
> > +	return STATUS_SUCCESS;
> > +}
> > +
> >  dev_t devnm2devid(char *devnm)
> >  {
> >  	/* First look in /sys/block/$DEVNM/dev for %d:%d  
>