| Message ID | 1593117741-28750-2-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | Fix muitpath/multipathd flush issue | expand |
On Thu, 2020-06-25 at 15:42 -0500, Benjamin Marzinski wrote: > dm_get_map() and dm_get_status() now use symbolic return codes. They > also differentiate between failing to get information from device- > mapper > and not finding the requested device. These symboilc return codes are > also used by update_multipath_* functions. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/devmapper.c | 51 +++++++++++++++++++++++++----------- > -- > libmultipath/devmapper.h | 6 +++++ > libmultipath/structs_vec.c | 45 +++++++++++++++++++-------------- > multipathd/main.c | 12 ++++----- > 4 files changed, 72 insertions(+), 42 deletions(-) > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > index 27d52398..f5cfe296 100644 > --- a/libmultipath/devmapper.c > +++ b/libmultipath/devmapper.c > @@ -534,36 +534,43 @@ int dm_map_present(const char * str) > > int dm_get_map(const char *name, unsigned long long *size, char > *outparams) > { > - int r = 1; > + int r = DMP_ERR; > struct dm_task *dmt; > uint64_t start, length; > char *target_type = NULL; > char *params = NULL; > > if (!(dmt = libmp_dm_task_create(DM_DEVICE_TABLE))) > - return 1; > + return r; > > if (!dm_task_set_name(dmt, name)) > goto out; > > dm_task_no_open_count(dmt); > > - if (!dm_task_run(dmt)) > + errno = 0; > + if (!dm_task_run(dmt)) { > + if (errno == ENXIO) Don't you have to use dm_task_get_errno(dmt) here? errno might have been overwritten... if you are certain you don't need it, a comment explaining it would be helpful. > + r = DMP_FAIL; You've created generic names, which is good, but these are perhaps a bit too generic. What's the difference bewteen DMP_FAIL and DMP_ERR? I can derive it from your code, but it's not obvious from the retcode names, and thus doesn't help the reader much. I suggest to keep DMT_ERR to denote a "generic" error condition, and use e.g. DMP_NOTFOUND for the other case. > goto out; > + } > > + r = DMP_FAIL; > /* Fetch 1st target */ > - dm_get_next_target(dmt, NULL, &start, &length, > - &target_type, ¶ms); > + if (dm_get_next_target(dmt, NULL, &start, &length, > + &target_type, ¶ms) != NULL) > + /* more than one target */ > + goto out; > > if (size) > *size = length; > > if (!outparams) { > - r = 0; > + r = DMP_PASS; Nit: DMP_OK or DMP_SUCCESS would be more conventional for successful completion. "pass" sounds like something more specific to me, like having passed a test or a filter. > goto out; > } > if (snprintf(outparams, PARAMS_SIZE, "%s", params) <= > PARAMS_SIZE) > - r = 0; > + r = DMP_PASS; > out: > dm_task_destroy(dmt); > return r; > @@ -637,35 +644,45 @@ is_mpath_part(const char *part_name, const char > *map_name) > > int dm_get_status(const char *name, char *outstatus) > { > - int r = 1; > + int r = DMP_ERR; > struct dm_task *dmt; > uint64_t start, length; > char *target_type = NULL; > char *status = NULL; > > if (!(dmt = libmp_dm_task_create(DM_DEVICE_STATUS))) > - return 1; > + return r; > > if (!dm_task_set_name(dmt, name)) > goto out; > > dm_task_no_open_count(dmt); > > - if (!dm_task_run(dmt)) > + errno = 0; > + if (!dm_task_run(dmt)) { > + if (errno == ENXIO) > + r = DMP_FAIL; > goto out; > + } see above > > + r = DMP_FAIL; > /* Fetch 1st target */ > - dm_get_next_target(dmt, NULL, &start, &length, > - &target_type, &status); > + if (dm_get_next_target(dmt, NULL, &start, &length, > + &target_type, &status) != NULL) > + goto out; > + > + if (!target_type || strcmp(target_type, TGT_MPATH) != 0) > + goto out; > + > if (!status) { > condlog(2, "get null status."); > goto out; > } > > if (snprintf(outstatus, PARAMS_SIZE, "%s", status) <= > PARAMS_SIZE) > - r = 0; > + r = DMP_PASS; > out: > - if (r) > + if (r != DMP_PASS) > condlog(0, "%s: error getting map status string", > name); > > dm_task_destroy(dmt); > @@ -920,7 +937,7 @@ int _dm_flush_map (const char * mapname, int > need_sync, int deferred_remove, > return 1; > > if (need_suspend && > - !dm_get_map(mapname, &mapsize, params) && > + dm_get_map(mapname, &mapsize, params) == DMP_PASS && > strstr(params, "queue_if_no_path")) { > if (!dm_queue_if_no_path(mapname, 0)) > queue_if_no_path = 1; > @@ -1129,7 +1146,7 @@ struct multipath *dm_get_multipath(const char > *name) > if (!mpp->alias) > goto out; > > - if (dm_get_map(name, &mpp->size, NULL)) > + if (dm_get_map(name, &mpp->size, NULL) != DMP_PASS) > goto out; > > dm_get_uuid(name, mpp->wwid, WWID_SIZE); > @@ -1313,7 +1330,7 @@ do_foreach_partmaps (const char * mapname, > /* > * and we can fetch the map table from the kernel > */ > - !dm_get_map(names->name, &size, ¶ms[0]) && > + dm_get_map(names->name, &size, ¶ms[0]) == > DMP_PASS && > > /* > * and the table maps over the multipath map > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h > index 5ed7edc5..5b18bf4b 100644 > --- a/libmultipath/devmapper.h > +++ b/libmultipath/devmapper.h > @@ -27,6 +27,12 @@ > #define UUID_PREFIX "mpath-" > #define UUID_PREFIX_LEN (sizeof(UUID_PREFIX) - 1) > > +enum { > + DMP_ERR = -1, > + DMP_PASS, > + DMP_FAIL, > +}; > + Nit: Why use both negative and positive numbers? It's not important, but I feel that unless we really want to treat DM_ERR in a very special way, it would be better to use only positive values. (Actually, if we go for some generalized retcode convention some day, we might save negative return values for standard libc errno values such as -ENOENT and use positive values for or own specific ones). (We can change the numeric values later of course). Cheers, Martin > void dm_init(int verbosity); > int dm_prereq(unsigned int *v); > void skip_libmp_dm_init(void); > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > index 077f2e42..2225abeb 100644 > --- a/libmultipath/structs_vec.c > +++ b/libmultipath/structs_vec.c > @@ -196,43 +196,47 @@ extract_hwe_from_path(struct multipath * mpp) > int > update_multipath_table (struct multipath *mpp, vector pathvec, int > is_daemon) > { > + int r = DMP_ERR; > char params[PARAMS_SIZE] = {0}; > > if (!mpp) > - return 1; > + return r; > > - if (dm_get_map(mpp->alias, &mpp->size, params)) { > - condlog(3, "%s: cannot get map", mpp->alias); > - return 1; > + r = dm_get_map(mpp->alias, &mpp->size, params); > + if (r != DMP_PASS) { > + condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error > getting table" : "map not present"); > + return r; > } > > if (disassemble_map(pathvec, params, mpp, is_daemon)) { > condlog(3, "%s: cannot disassemble map", mpp->alias); > - return 1; > + return DMP_ERR; > } > > - return 0; > + return DMP_PASS; > } > > int > update_multipath_status (struct multipath *mpp) > { > + int r = DMP_ERR; > char status[PARAMS_SIZE] = {0}; > > if (!mpp) > - return 1; > + return r; > > - if (dm_get_status(mpp->alias, status)) { > - condlog(3, "%s: cannot get status", mpp->alias); > - return 1; > + r = dm_get_status(mpp->alias, status); > + if (r != DMP_PASS) { > + condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error > getting status" : "map not present"); > + return r; > } > > if (disassemble_status(status, mpp)) { > condlog(3, "%s: cannot disassemble status", mpp- > >alias); > - return 1; > + return DMP_ERR; > } > > - return 0; > + return DMP_PASS; > } > > void sync_paths(struct multipath *mpp, vector pathvec) > @@ -264,10 +268,10 @@ int > update_multipath_strings(struct multipath *mpp, vector pathvec, int > is_daemon) > { > struct pathgroup *pgp; > - int i; > + int i, r = DMP_ERR; > > if (!mpp) > - return 1; > + return r; > > update_mpp_paths(mpp, pathvec); > condlog(4, "%s: %s", mpp->alias, __FUNCTION__); > @@ -276,18 +280,21 @@ update_multipath_strings(struct multipath *mpp, > vector pathvec, int is_daemon) > free_pgvec(mpp->pg, KEEP_PATHS); > mpp->pg = NULL; > > - if (update_multipath_table(mpp, pathvec, is_daemon)) > - return 1; > + r = update_multipath_table(mpp, pathvec, is_daemon); > + if (r != DMP_PASS) > + return r; > + > sync_paths(mpp, pathvec); > > - if (update_multipath_status(mpp)) > - return 1; > + r = update_multipath_status(mpp); > + if (r != DMP_PASS) > + return r; > > vector_foreach_slot(mpp->pg, pgp, i) > if (pgp->paths) > path_group_prio_update(pgp); > > - return 0; > + return DMP_PASS; > } > > static void enter_recovery_mode(struct multipath *mpp) > diff --git a/multipathd/main.c b/multipathd/main.c > index 205ddb32..d73b1b9a 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -418,7 +418,7 @@ int __setup_multipath(struct vectors *vecs, > struct multipath *mpp, > goto out; > } > > - if (update_multipath_strings(mpp, vecs->pathvec, 1)) { > + if (update_multipath_strings(mpp, vecs->pathvec, 1) != > DMP_PASS) { > condlog(0, "%s: failed to setup multipath", mpp- > >alias); > goto out; > } > @@ -557,9 +557,9 @@ add_map_without_path (struct vectors *vecs, const > char *alias) > mpp->mpe = find_mpe(conf->mptable, mpp->wwid); > put_multipath_config(conf); > > - if (update_multipath_table(mpp, vecs->pathvec, 1)) > + if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_PASS) > goto out; > - if (update_multipath_status(mpp)) > + if (update_multipath_status(mpp) != DMP_PASS) > goto out; > > if (!vector_alloc_slot(vecs->mpvec)) > @@ -1350,8 +1350,8 @@ map_discovery (struct vectors * vecs) > return 1; > > vector_foreach_slot (vecs->mpvec, mpp, i) > - if (update_multipath_table(mpp, vecs->pathvec, 1) || > - update_multipath_status(mpp)) { > + if (update_multipath_table(mpp, vecs->pathvec, 1) != > DMP_PASS || > + update_multipath_status(mpp) != DMP_PASS) { > remove_map(mpp, vecs, 1); > i--; > } > @@ -2091,7 +2091,7 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > /* > * Synchronize with kernel state > */ > - if (update_multipath_strings(pp->mpp, vecs->pathvec, 1)) { > + if (update_multipath_strings(pp->mpp, vecs->pathvec, 1) != > DMP_PASS) { > condlog(1, "%s: Could not synchronize with kernel > state", > pp->dev); > pp->dmstate = PSTATE_UNDEF;
On Wed, Jul 01, 2020 at 08:15:49PM +0000, Martin Wilck wrote: > On Thu, 2020-06-25 at 15:42 -0500, Benjamin Marzinski wrote: > > dm_get_map() and dm_get_status() now use symbolic return codes. They > > also differentiate between failing to get information from device- > > mapper > > and not finding the requested device. These symboilc return codes are > > also used by update_multipath_* functions. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/devmapper.c | 51 +++++++++++++++++++++++++----------- > > -- > > libmultipath/devmapper.h | 6 +++++ > > libmultipath/structs_vec.c | 45 +++++++++++++++++++-------------- > > multipathd/main.c | 12 ++++----- > > 4 files changed, 72 insertions(+), 42 deletions(-) > > > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > > index 27d52398..f5cfe296 100644 > > --- a/libmultipath/devmapper.c > > +++ b/libmultipath/devmapper.c > > @@ -534,36 +534,43 @@ int dm_map_present(const char * str) > > > > int dm_get_map(const char *name, unsigned long long *size, char > > *outparams) > > { > > - int r = 1; > > + int r = DMP_ERR; > > struct dm_task *dmt; > > uint64_t start, length; > > char *target_type = NULL; > > char *params = NULL; > > > > if (!(dmt = libmp_dm_task_create(DM_DEVICE_TABLE))) > > - return 1; > > + return r; > > > > if (!dm_task_set_name(dmt, name)) > > goto out; > > > > dm_task_no_open_count(dmt); > > > > - if (!dm_task_run(dmt)) > > + errno = 0; > > + if (!dm_task_run(dmt)) { > > + if (errno == ENXIO) > > Don't you have to use dm_task_get_errno(dmt) here? > errno might have been overwritten... if you are certain you don't need > it, a comment explaining it would be helpful. Err.. I didn't know that existed. Sure I can use it, and your other suggestions are reasonable as well -Ben > > > > + r = DMP_FAIL; > > You've created generic names, which is good, but these are perhaps a > bit too generic. What's the difference bewteen DMP_FAIL and DMP_ERR? I > can derive it from your code, but it's not obvious from the retcode > names, and thus doesn't help the reader much. I suggest to keep DMT_ERR > to denote a "generic" error condition, and use e.g. DMP_NOTFOUND for > the other case. > > > goto out; > > + } > > > > + r = DMP_FAIL; > > /* Fetch 1st target */ > > - dm_get_next_target(dmt, NULL, &start, &length, > > - &target_type, ¶ms); > > + if (dm_get_next_target(dmt, NULL, &start, &length, > > + &target_type, ¶ms) != NULL) > > + /* more than one target */ > > + goto out; > > > > if (size) > > *size = length; > > > > if (!outparams) { > > - r = 0; > > + r = DMP_PASS; > > Nit: DMP_OK or DMP_SUCCESS would be more conventional for successful > completion. "pass" sounds like something more specific to me, > like having passed a test or a filter. > > > goto out; > > } > > if (snprintf(outparams, PARAMS_SIZE, "%s", params) <= > > PARAMS_SIZE) > > - r = 0; > > + r = DMP_PASS; > > out: > > dm_task_destroy(dmt); > > return r; > > @@ -637,35 +644,45 @@ is_mpath_part(const char *part_name, const char > > *map_name) > > > > int dm_get_status(const char *name, char *outstatus) > > { > > - int r = 1; > > + int r = DMP_ERR; > > struct dm_task *dmt; > > uint64_t start, length; > > char *target_type = NULL; > > char *status = NULL; > > > > if (!(dmt = libmp_dm_task_create(DM_DEVICE_STATUS))) > > - return 1; > > + return r; > > > > if (!dm_task_set_name(dmt, name)) > > goto out; > > > > dm_task_no_open_count(dmt); > > > > - if (!dm_task_run(dmt)) > > + errno = 0; > > + if (!dm_task_run(dmt)) { > > + if (errno == ENXIO) > > + r = DMP_FAIL; > > goto out; > > + } > > see above > > > > > + r = DMP_FAIL; > > /* Fetch 1st target */ > > - dm_get_next_target(dmt, NULL, &start, &length, > > - &target_type, &status); > > + if (dm_get_next_target(dmt, NULL, &start, &length, > > + &target_type, &status) != NULL) > > + goto out; > > + > > + if (!target_type || strcmp(target_type, TGT_MPATH) != 0) > > + goto out; > > + > > if (!status) { > > condlog(2, "get null status."); > > goto out; > > } > > > > if (snprintf(outstatus, PARAMS_SIZE, "%s", status) <= > > PARAMS_SIZE) > > - r = 0; > > + r = DMP_PASS; > > out: > > - if (r) > > + if (r != DMP_PASS) > > condlog(0, "%s: error getting map status string", > > name); > > > > dm_task_destroy(dmt); > > @@ -920,7 +937,7 @@ int _dm_flush_map (const char * mapname, int > > need_sync, int deferred_remove, > > return 1; > > > > if (need_suspend && > > - !dm_get_map(mapname, &mapsize, params) && > > + dm_get_map(mapname, &mapsize, params) == DMP_PASS && > > strstr(params, "queue_if_no_path")) { > > if (!dm_queue_if_no_path(mapname, 0)) > > queue_if_no_path = 1; > > @@ -1129,7 +1146,7 @@ struct multipath *dm_get_multipath(const char > > *name) > > if (!mpp->alias) > > goto out; > > > > - if (dm_get_map(name, &mpp->size, NULL)) > > + if (dm_get_map(name, &mpp->size, NULL) != DMP_PASS) > > goto out; > > > > dm_get_uuid(name, mpp->wwid, WWID_SIZE); > > @@ -1313,7 +1330,7 @@ do_foreach_partmaps (const char * mapname, > > /* > > * and we can fetch the map table from the kernel > > */ > > - !dm_get_map(names->name, &size, ¶ms[0]) && > > + dm_get_map(names->name, &size, ¶ms[0]) == > > DMP_PASS && > > > > /* > > * and the table maps over the multipath map > > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h > > index 5ed7edc5..5b18bf4b 100644 > > --- a/libmultipath/devmapper.h > > +++ b/libmultipath/devmapper.h > > @@ -27,6 +27,12 @@ > > #define UUID_PREFIX "mpath-" > > #define UUID_PREFIX_LEN (sizeof(UUID_PREFIX) - 1) > > > > +enum { > > + DMP_ERR = -1, > > + DMP_PASS, > > + DMP_FAIL, > > +}; > > + > > Nit: Why use both negative and positive numbers? It's not important, > but I feel that unless we really want to treat DM_ERR in a very special > way, it would be better to use only positive values. (Actually, if we > go for some generalized retcode convention some day, we might save > negative return values for standard libc errno values such > as -ENOENT and use positive values for or own specific ones). > > (We can change the numeric values later of course). > > Cheers, > Martin > > > void dm_init(int verbosity); > > int dm_prereq(unsigned int *v); > > void skip_libmp_dm_init(void); > > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > > index 077f2e42..2225abeb 100644 > > --- a/libmultipath/structs_vec.c > > +++ b/libmultipath/structs_vec.c > > @@ -196,43 +196,47 @@ extract_hwe_from_path(struct multipath * mpp) > > int > > update_multipath_table (struct multipath *mpp, vector pathvec, int > > is_daemon) > > { > > + int r = DMP_ERR; > > char params[PARAMS_SIZE] = {0}; > > > > if (!mpp) > > - return 1; > > + return r; > > > > - if (dm_get_map(mpp->alias, &mpp->size, params)) { > > - condlog(3, "%s: cannot get map", mpp->alias); > > - return 1; > > + r = dm_get_map(mpp->alias, &mpp->size, params); > > + if (r != DMP_PASS) { > > + condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error > > getting table" : "map not present"); > > + return r; > > } > > > > if (disassemble_map(pathvec, params, mpp, is_daemon)) { > > condlog(3, "%s: cannot disassemble map", mpp->alias); > > - return 1; > > + return DMP_ERR; > > } > > > > - return 0; > > + return DMP_PASS; > > } > > > > int > > update_multipath_status (struct multipath *mpp) > > { > > + int r = DMP_ERR; > > char status[PARAMS_SIZE] = {0}; > > > > if (!mpp) > > - return 1; > > + return r; > > > > - if (dm_get_status(mpp->alias, status)) { > > - condlog(3, "%s: cannot get status", mpp->alias); > > - return 1; > > + r = dm_get_status(mpp->alias, status); > > + if (r != DMP_PASS) { > > + condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error > > getting status" : "map not present"); > > + return r; > > } > > > > if (disassemble_status(status, mpp)) { > > condlog(3, "%s: cannot disassemble status", mpp- > > >alias); > > - return 1; > > + return DMP_ERR; > > } > > > > - return 0; > > + return DMP_PASS; > > } > > > > void sync_paths(struct multipath *mpp, vector pathvec) > > @@ -264,10 +268,10 @@ int > > update_multipath_strings(struct multipath *mpp, vector pathvec, int > > is_daemon) > > { > > struct pathgroup *pgp; > > - int i; > > + int i, r = DMP_ERR; > > > > if (!mpp) > > - return 1; > > + return r; > > > > update_mpp_paths(mpp, pathvec); > > condlog(4, "%s: %s", mpp->alias, __FUNCTION__); > > @@ -276,18 +280,21 @@ update_multipath_strings(struct multipath *mpp, > > vector pathvec, int is_daemon) > > free_pgvec(mpp->pg, KEEP_PATHS); > > mpp->pg = NULL; > > > > - if (update_multipath_table(mpp, pathvec, is_daemon)) > > - return 1; > > + r = update_multipath_table(mpp, pathvec, is_daemon); > > + if (r != DMP_PASS) > > + return r; > > + > > sync_paths(mpp, pathvec); > > > > - if (update_multipath_status(mpp)) > > - return 1; > > + r = update_multipath_status(mpp); > > + if (r != DMP_PASS) > > + return r; > > > > vector_foreach_slot(mpp->pg, pgp, i) > > if (pgp->paths) > > path_group_prio_update(pgp); > > > > - return 0; > > + return DMP_PASS; > > } > > > > static void enter_recovery_mode(struct multipath *mpp) > > diff --git a/multipathd/main.c b/multipathd/main.c > > index 205ddb32..d73b1b9a 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -418,7 +418,7 @@ int __setup_multipath(struct vectors *vecs, > > struct multipath *mpp, > > goto out; > > } > > > > - if (update_multipath_strings(mpp, vecs->pathvec, 1)) { > > + if (update_multipath_strings(mpp, vecs->pathvec, 1) != > > DMP_PASS) { > > condlog(0, "%s: failed to setup multipath", mpp- > > >alias); > > goto out; > > } > > @@ -557,9 +557,9 @@ add_map_without_path (struct vectors *vecs, const > > char *alias) > > mpp->mpe = find_mpe(conf->mptable, mpp->wwid); > > put_multipath_config(conf); > > > > - if (update_multipath_table(mpp, vecs->pathvec, 1)) > > + if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_PASS) > > goto out; > > - if (update_multipath_status(mpp)) > > + if (update_multipath_status(mpp) != DMP_PASS) > > goto out; > > > > if (!vector_alloc_slot(vecs->mpvec)) > > @@ -1350,8 +1350,8 @@ map_discovery (struct vectors * vecs) > > return 1; > > > > vector_foreach_slot (vecs->mpvec, mpp, i) > > - if (update_multipath_table(mpp, vecs->pathvec, 1) || > > - update_multipath_status(mpp)) { > > + if (update_multipath_table(mpp, vecs->pathvec, 1) != > > DMP_PASS || > > + update_multipath_status(mpp) != DMP_PASS) { > > remove_map(mpp, vecs, 1); > > i--; > > } > > @@ -2091,7 +2091,7 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > /* > > * Synchronize with kernel state > > */ > > - if (update_multipath_strings(pp->mpp, vecs->pathvec, 1)) { > > + if (update_multipath_strings(pp->mpp, vecs->pathvec, 1) != > > DMP_PASS) { > > condlog(1, "%s: Could not synchronize with kernel > > state", > > pp->dev); > > pp->dmstate = PSTATE_UNDEF; > > -- > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107 > SUSE Software Solutions Germany GmbH > HRB 36809, AG Nürnberg GF: Felix > Imendörffer > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 27d52398..f5cfe296 100644 --- a/libmultipath/devmapper.c +++ b/libmultipath/devmapper.c @@ -534,36 +534,43 @@ int dm_map_present(const char * str) int dm_get_map(const char *name, unsigned long long *size, char *outparams) { - int r = 1; + int r = DMP_ERR; struct dm_task *dmt; uint64_t start, length; char *target_type = NULL; char *params = NULL; if (!(dmt = libmp_dm_task_create(DM_DEVICE_TABLE))) - return 1; + return r; if (!dm_task_set_name(dmt, name)) goto out; dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) + errno = 0; + if (!dm_task_run(dmt)) { + if (errno == ENXIO) + r = DMP_FAIL; goto out; + } + r = DMP_FAIL; /* Fetch 1st target */ - dm_get_next_target(dmt, NULL, &start, &length, - &target_type, ¶ms); + if (dm_get_next_target(dmt, NULL, &start, &length, + &target_type, ¶ms) != NULL) + /* more than one target */ + goto out; if (size) *size = length; if (!outparams) { - r = 0; + r = DMP_PASS; goto out; } if (snprintf(outparams, PARAMS_SIZE, "%s", params) <= PARAMS_SIZE) - r = 0; + r = DMP_PASS; out: dm_task_destroy(dmt); return r; @@ -637,35 +644,45 @@ is_mpath_part(const char *part_name, const char *map_name) int dm_get_status(const char *name, char *outstatus) { - int r = 1; + int r = DMP_ERR; struct dm_task *dmt; uint64_t start, length; char *target_type = NULL; char *status = NULL; if (!(dmt = libmp_dm_task_create(DM_DEVICE_STATUS))) - return 1; + return r; if (!dm_task_set_name(dmt, name)) goto out; dm_task_no_open_count(dmt); - if (!dm_task_run(dmt)) + errno = 0; + if (!dm_task_run(dmt)) { + if (errno == ENXIO) + r = DMP_FAIL; goto out; + } + r = DMP_FAIL; /* Fetch 1st target */ - dm_get_next_target(dmt, NULL, &start, &length, - &target_type, &status); + if (dm_get_next_target(dmt, NULL, &start, &length, + &target_type, &status) != NULL) + goto out; + + if (!target_type || strcmp(target_type, TGT_MPATH) != 0) + goto out; + if (!status) { condlog(2, "get null status."); goto out; } if (snprintf(outstatus, PARAMS_SIZE, "%s", status) <= PARAMS_SIZE) - r = 0; + r = DMP_PASS; out: - if (r) + if (r != DMP_PASS) condlog(0, "%s: error getting map status string", name); dm_task_destroy(dmt); @@ -920,7 +937,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove, return 1; if (need_suspend && - !dm_get_map(mapname, &mapsize, params) && + dm_get_map(mapname, &mapsize, params) == DMP_PASS && strstr(params, "queue_if_no_path")) { if (!dm_queue_if_no_path(mapname, 0)) queue_if_no_path = 1; @@ -1129,7 +1146,7 @@ struct multipath *dm_get_multipath(const char *name) if (!mpp->alias) goto out; - if (dm_get_map(name, &mpp->size, NULL)) + if (dm_get_map(name, &mpp->size, NULL) != DMP_PASS) goto out; dm_get_uuid(name, mpp->wwid, WWID_SIZE); @@ -1313,7 +1330,7 @@ do_foreach_partmaps (const char * mapname, /* * and we can fetch the map table from the kernel */ - !dm_get_map(names->name, &size, ¶ms[0]) && + dm_get_map(names->name, &size, ¶ms[0]) == DMP_PASS && /* * and the table maps over the multipath map diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h index 5ed7edc5..5b18bf4b 100644 --- a/libmultipath/devmapper.h +++ b/libmultipath/devmapper.h @@ -27,6 +27,12 @@ #define UUID_PREFIX "mpath-" #define UUID_PREFIX_LEN (sizeof(UUID_PREFIX) - 1) +enum { + DMP_ERR = -1, + DMP_PASS, + DMP_FAIL, +}; + void dm_init(int verbosity); int dm_prereq(unsigned int *v); void skip_libmp_dm_init(void); diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index 077f2e42..2225abeb 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -196,43 +196,47 @@ extract_hwe_from_path(struct multipath * mpp) int update_multipath_table (struct multipath *mpp, vector pathvec, int is_daemon) { + int r = DMP_ERR; char params[PARAMS_SIZE] = {0}; if (!mpp) - return 1; + return r; - if (dm_get_map(mpp->alias, &mpp->size, params)) { - condlog(3, "%s: cannot get map", mpp->alias); - return 1; + r = dm_get_map(mpp->alias, &mpp->size, params); + if (r != DMP_PASS) { + condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting table" : "map not present"); + return r; } if (disassemble_map(pathvec, params, mpp, is_daemon)) { condlog(3, "%s: cannot disassemble map", mpp->alias); - return 1; + return DMP_ERR; } - return 0; + return DMP_PASS; } int update_multipath_status (struct multipath *mpp) { + int r = DMP_ERR; char status[PARAMS_SIZE] = {0}; if (!mpp) - return 1; + return r; - if (dm_get_status(mpp->alias, status)) { - condlog(3, "%s: cannot get status", mpp->alias); - return 1; + r = dm_get_status(mpp->alias, status); + if (r != DMP_PASS) { + condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting status" : "map not present"); + return r; } if (disassemble_status(status, mpp)) { condlog(3, "%s: cannot disassemble status", mpp->alias); - return 1; + return DMP_ERR; } - return 0; + return DMP_PASS; } void sync_paths(struct multipath *mpp, vector pathvec) @@ -264,10 +268,10 @@ int update_multipath_strings(struct multipath *mpp, vector pathvec, int is_daemon) { struct pathgroup *pgp; - int i; + int i, r = DMP_ERR; if (!mpp) - return 1; + return r; update_mpp_paths(mpp, pathvec); condlog(4, "%s: %s", mpp->alias, __FUNCTION__); @@ -276,18 +280,21 @@ update_multipath_strings(struct multipath *mpp, vector pathvec, int is_daemon) free_pgvec(mpp->pg, KEEP_PATHS); mpp->pg = NULL; - if (update_multipath_table(mpp, pathvec, is_daemon)) - return 1; + r = update_multipath_table(mpp, pathvec, is_daemon); + if (r != DMP_PASS) + return r; + sync_paths(mpp, pathvec); - if (update_multipath_status(mpp)) - return 1; + r = update_multipath_status(mpp); + if (r != DMP_PASS) + return r; vector_foreach_slot(mpp->pg, pgp, i) if (pgp->paths) path_group_prio_update(pgp); - return 0; + return DMP_PASS; } static void enter_recovery_mode(struct multipath *mpp) diff --git a/multipathd/main.c b/multipathd/main.c index 205ddb32..d73b1b9a 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -418,7 +418,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp, goto out; } - if (update_multipath_strings(mpp, vecs->pathvec, 1)) { + if (update_multipath_strings(mpp, vecs->pathvec, 1) != DMP_PASS) { condlog(0, "%s: failed to setup multipath", mpp->alias); goto out; } @@ -557,9 +557,9 @@ add_map_without_path (struct vectors *vecs, const char *alias) mpp->mpe = find_mpe(conf->mptable, mpp->wwid); put_multipath_config(conf); - if (update_multipath_table(mpp, vecs->pathvec, 1)) + if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_PASS) goto out; - if (update_multipath_status(mpp)) + if (update_multipath_status(mpp) != DMP_PASS) goto out; if (!vector_alloc_slot(vecs->mpvec)) @@ -1350,8 +1350,8 @@ map_discovery (struct vectors * vecs) return 1; vector_foreach_slot (vecs->mpvec, mpp, i) - if (update_multipath_table(mpp, vecs->pathvec, 1) || - update_multipath_status(mpp)) { + if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_PASS || + update_multipath_status(mpp) != DMP_PASS) { remove_map(mpp, vecs, 1); i--; } @@ -2091,7 +2091,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) /* * Synchronize with kernel state */ - if (update_multipath_strings(pp->mpp, vecs->pathvec, 1)) { + if (update_multipath_strings(pp->mpp, vecs->pathvec, 1) != DMP_PASS) { condlog(1, "%s: Could not synchronize with kernel state", pp->dev); pp->dmstate = PSTATE_UNDEF;
dm_get_map() and dm_get_status() now use symbolic return codes. They also differentiate between failing to get information from device-mapper and not finding the requested device. These symboilc return codes are also used by update_multipath_* functions. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/devmapper.c | 51 +++++++++++++++++++++++++------------- libmultipath/devmapper.h | 6 +++++ libmultipath/structs_vec.c | 45 +++++++++++++++++++-------------- multipathd/main.c | 12 ++++----- 4 files changed, 72 insertions(+), 42 deletions(-)