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@xxxxxxxxxxxxxxx>
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.
I would prefer if we move towards standard POSIX error codes instead of
trying to invent new ones.
Thanks,
Jes
> ---
> 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
