d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Clarify when GEOM utilities exit with success or failure.

Browse Source 
Historically, GEOM utilities (gpart(8), gstripe(8), gmirror(8),
etc) used the gctl_error() routine to report errors. If they called
gctl_error() they would exit with EXIT_FAILURE, otherwise they would
return with EXIT_SUCCESS. If they used gctl_error() to output an
informational message, for example when run with the -v (verbose)
option, they would mistakenly exit with EXIT_FAILURE. A further
limitation of the gctl_error() function was that it could only be
called once. Messages from any additional calls to gctl_error()
would be silently discarded.

To resolve these problems a new function, gctl_msg() has been added.
It can be called multiple times to output multiple messages. It
also has an additional errno argument which should be zero if it is
an informational message or an errno value (EINVAL, EBUSY, etc) if
it is an error. When done the gctl_post_messages() function should
be called to indicate that all messages have been posted. If any
of the messages had a non-zero errno, the utility will EXIT_FAILURE.
If only informational messages (with zero errno) were posted, the
utility will EXIT_SUCCESS.

Tested by:   Peter Holm
PR:          265184
MFC after:   1 week
This commit is contained in:
Kirk McKusick 2022-07-16 10:25:22 -07:00
parent 9917049b60
commit 90e29718cf
5 changed files with 57 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
lib/libgeom/geom_ctl.c
View File

@ -194,7 +194,7 @@ gctl_rw_param(struct gctl_req *req, const char *name, int len, void *value)
const char * const char *
gctl_issue(struct gctl_req *req) gctl_issue(struct gctl_req *req)
{ {
int fd, error; int fd;
if (req == NULL) if (req == NULL)
return ("NULL request pointer"); return ("NULL request pointer");
@ -212,11 +212,11 @@ gctl_issue(struct gctl_req *req)
fd = open(_PATH_DEV PATH_GEOM_CTL, O_RDONLY); fd = open(_PATH_DEV PATH_GEOM_CTL, O_RDONLY);
if (fd < 0) if (fd < 0)
return(strerror(errno)); return(strerror(errno));
error = ioctl(fd, GEOM_CTL, req); req->nerror = ioctl(fd, GEOM_CTL, req);
close(fd); close(fd);
if (req->error[0] != '\0') if (req->error[0] != '\0')
return (req->error); return (req->error);
if (error != 0) if (req->nerror == -1)
return(strerror(errno)); return(strerror(errno));
return (NULL); return (NULL);
} }

5
sbin/geom/core/geom.c
View File

@ -503,7 +503,10 @@ run_command(int argc, char *argv[])
} }
if (errstr != NULL && errstr[0] != '\0') { if (errstr != NULL && errstr[0] != '\0') {
warnx("%s", errstr); warnx("%s", errstr);
if (strncmp(errstr, "warning: ", strlen("warning: ")) != 0) { /* Suppress EXIT_FAILURE for warnings */
if (strncmp(errstr, "warning: ", strlen("warning: ")) == 0)
req->nerror = 0;
if (req->nerror != 0) {
gctl_free(req); gctl_free(req);
exit(EXIT_FAILURE); exit(EXIT_FAILURE);
} }

2
sys/geom/geom.h
View File

@ -436,7 +436,7 @@ char const *gctl_get_asciiparam(struct gctl_req *req, const char *param);
void *gctl_get_paraml(struct gctl_req *req, const char *param, int len); void *gctl_get_paraml(struct gctl_req *req, const char *param, int len);
void *gctl_get_paraml_opt(struct gctl_req *req, const char *param, int len); void *gctl_get_paraml_opt(struct gctl_req *req, const char *param, int len);
int gctl_error(struct gctl_req *req, const char *fmt, ...) __printflike(2, 3); int gctl_error(struct gctl_req *req, const char *fmt, ...) __printflike(2, 3);
void gctl_msg(struct gctl_req *req, const char *fmt, ...) __printflike(2, 3); void gctl_msg(struct gctl_req *req, int, const char *fmt, ...) __printflike(3, 4);
void gctl_post_messages(struct gctl_req *req); void gctl_post_messages(struct gctl_req *req);
struct g_class *gctl_get_class(struct gctl_req *req, char const *arg); struct g_class *gctl_get_class(struct gctl_req *req, char const *arg);
struct g_geom *gctl_get_geom(struct gctl_req *req, struct g_class *mp, char const *arg); struct g_geom *gctl_get_geom(struct gctl_req *req, struct g_class *mp, char const *arg);

10
sys/geom/geom_ctl.c
View File

@ -117,9 +117,15 @@ gctl_error(struct gctl_req *req, const char *fmt, ...)
* the application must either call gctl_post_messages() or * the application must either call gctl_post_messages() or
* call gctl_error() to cause the messages to be reported to * call gctl_error() to cause the messages to be reported to
* the calling process. * the calling process.
*
* The errno argument should be zero if it is an informational
* message or an errno value (EINVAL, EBUSY, etc) if it is an error.
* If any of the messages has a non-zero errno, the utility will
* EXIT_FAILURE. If only informational messages (with zero errno)
* are posted, the utility will EXIT_SUCCESS.
*/ */
void void
gctl_msg(struct gctl_req *req, const char *fmt, ...) gctl_msg(struct gctl_req *req, int errno, const char *fmt, ...)
{ {
va_list ap; va_list ap;
@ -131,6 +137,8 @@ gctl_msg(struct gctl_req *req, const char *fmt, ...)
#endif #endif
return; return;
} }
if (req->nerror == 0)
req->nerror = errno;
/* Put second and later messages indented on a new line */ /* Put second and later messages indented on a new line */
if (sbuf_len(req->serror) > 0) if (sbuf_len(req->serror) > 0)
sbuf_cat(req->serror, "\n\t"); sbuf_cat(req->serror, "\n\t");

75
sys/geom/union/g_union.c
View File

@ -341,8 +341,9 @@ g_union_ctl_create(struct gctl_req *req, struct g_class *mp, bool verbose)
(sc->sc_root_size + sc->sc_root_size * sc->sc_leaf_size) * (sc->sc_root_size + sc->sc_root_size * sc->sc_leaf_size) *
sizeof(uint64_t) + roundup(sc->sc_root_size, BITS_PER_ENTRY); sizeof(uint64_t) + roundup(sc->sc_root_size, BITS_PER_ENTRY);
if (verbose) if (verbose)
gctl_error(req, "Device %s created with memory map size %jd.", gctl_msg(req, 0, "Device %s created with memory map size %jd.",
gp->name, (intmax_t)sc->sc_writemap_memory); gp->name, (intmax_t)sc->sc_writemap_memory);
gctl_post_messages(req);
G_UNION_DEBUG(1, "Device %s created with memory map size %jd.", G_UNION_DEBUG(1, "Device %s created with memory map size %jd.",
gp->name, (intmax_t)sc->sc_writemap_memory); gp->name, (intmax_t)sc->sc_writemap_memory);
return; return;
@ -373,7 +374,8 @@ g_union_fetcharg(struct gctl_req *req, const char *name)
return (0); return (0);
if (*val >= 0) if (*val >= 0)
return (*val); return (*val);
gctl_error(req, "Invalid '%s': negative value, using default.", name); gctl_msg(req, EINVAL, "Invalid '%s' (%jd): negative value, "
"using default.", name, *val);
return (0); return (0);
} }
@ -425,20 +427,20 @@ g_union_ctl_destroy(struct gctl_req *req, struct g_class *mp, bool verbose)
snprintf(param, sizeof(param), "arg%d", i); snprintf(param, sizeof(param), "arg%d", i);
name = gctl_get_asciiparam(req, param); name = gctl_get_asciiparam(req, param);
if (name == NULL) { if (name == NULL) {
gctl_msg(req, "No '%s' argument.", param); gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue; continue;
} }
if (strncmp(name, _PATH_DEV, strlen(_PATH_DEV)) == 0) if (strncmp(name, _PATH_DEV, strlen(_PATH_DEV)) == 0)
name += strlen(_PATH_DEV); name += strlen(_PATH_DEV);
gp = g_union_find_geom(mp, name); gp = g_union_find_geom(mp, name);
if (gp == NULL) { if (gp == NULL) {
gctl_msg(req, "Device %s is invalid.", name); gctl_msg(req, EINVAL, "Device %s is invalid.", name);
continue; continue;
} }
error = g_union_destroy(verbose ? req : NULL, gp, *force); error = g_union_destroy(verbose ? req : NULL, gp, *force);
if (error != 0) if (error != 0)
gctl_msg(req, "Error %d: cannot destroy device %s.", gctl_msg(req, error, "Error %d: "
error, gp->name); "cannot destroy device %s.", error, gp->name);
} }
gctl_post_messages(req); gctl_post_messages(req);
} }
@ -486,12 +488,12 @@ g_union_ctl_reset(struct gctl_req *req, struct g_class *mp, bool verbose)
snprintf(param, sizeof(param), "arg%d", i); snprintf(param, sizeof(param), "arg%d", i);
pp = gctl_get_provider(req, param); pp = gctl_get_provider(req, param);
if (pp == NULL) { if (pp == NULL) {
gctl_msg(req, "No '%s' argument.", param); gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue; continue;
} }
gp = pp->geom; gp = pp->geom;
if (gp->class != mp) { if (gp->class != mp) {
gctl_msg(req, "Provider %s is invalid.", gctl_msg(req, EINVAL, "Provider %s is invalid.",
pp->name); pp->name);
continue; continue;
} }
@ -508,7 +510,7 @@ g_union_ctl_reset(struct gctl_req *req, struct g_class *mp, bool verbose)
sc->sc_readbytes = 0; sc->sc_readbytes = 0;
sc->sc_wrotebytes = 0; sc->sc_wrotebytes = 0;
if (verbose) if (verbose)
gctl_msg(req, "Device %s has been reset.", pp->name); gctl_msg(req, 0, "Device %s has been reset.", pp->name);
G_UNION_DEBUG(1, "Device %s has been reset.", pp->name); G_UNION_DEBUG(1, "Device %s has been reset.", pp->name);
} }
gctl_post_messages(req); gctl_post_messages(req);
@ -542,17 +544,18 @@ g_union_ctl_revert(struct gctl_req *req, struct g_class *mp, bool verbose)
snprintf(param, sizeof(param), "arg%d", i); snprintf(param, sizeof(param), "arg%d", i);
pp = gctl_get_provider(req, param); pp = gctl_get_provider(req, param);
if (pp == NULL) { if (pp == NULL) {
gctl_msg(req, "No '%s' argument.", param); gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue; continue;
} }
gp = pp->geom; gp = pp->geom;
if (gp->class != mp) { if (gp->class != mp) {
gctl_msg(req, "Provider %s is invalid.", pp->name); gctl_msg(req, EINVAL, "Provider %s is invalid.",
pp->name);
continue; continue;
} }
sc = gp->softc; sc = gp->softc;
if (g_union_get_writelock(sc) != 0) { if (g_union_get_writelock(sc) != 0) {
gctl_msg(req, "Revert already in progress for " gctl_msg(req, EINVAL, "Revert already in progress for "
"provider %s.", pp->name); "provider %s.", pp->name);
continue; continue;
} }
@ -560,8 +563,8 @@ g_union_ctl_revert(struct gctl_req *req, struct g_class *mp, bool verbose)
* No mount or other use of union is allowed. * No mount or other use of union is allowed.
*/ */
if (pp->acr > 0 || pp->acw > 0 || pp->ace > 0) { if (pp->acr > 0 || pp->acw > 0 || pp->ace > 0) {
gctl_msg(req, "Unable to get exclusive access for " gctl_msg(req, EPERM, "Unable to get exclusive access "
"reverting of %s;\n\t%s cannot be mounted or " "for reverting of %s;\n\t%s cannot be mounted or "
"otherwise open during a revert.", "otherwise open during a revert.",
pp->name, pp->name); pp->name, pp->name);
g_union_rel_writelock(sc); g_union_rel_writelock(sc);
@ -570,7 +573,8 @@ g_union_ctl_revert(struct gctl_req *req, struct g_class *mp, bool verbose)
g_union_revert(sc); g_union_revert(sc);
g_union_rel_writelock(sc); g_union_rel_writelock(sc);
if (verbose) if (verbose)
gctl_msg(req, "Device %s has been reverted.", pp->name); gctl_msg(req, 0, "Device %s has been reverted.",
pp->name);
G_UNION_DEBUG(1, "Device %s has been reverted.", pp->name); G_UNION_DEBUG(1, "Device %s has been reverted.", pp->name);
} }
gctl_post_messages(req); gctl_post_messages(req);
@ -637,17 +641,18 @@ g_union_ctl_commit(struct gctl_req *req, struct g_class *mp, bool verbose)
snprintf(param, sizeof(param), "arg%d", i); snprintf(param, sizeof(param), "arg%d", i);
pp = gctl_get_provider(req, param); pp = gctl_get_provider(req, param);
if (pp == NULL) { if (pp == NULL) {
gctl_msg(req, "No '%s' argument.", param); gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue; continue;
} }
gp = pp->geom; gp = pp->geom;
if (gp->class != mp) { if (gp->class != mp) {
gctl_msg(req, "Provider %s is invalid.", pp->name); gctl_msg(req, EINVAL, "Provider %s is invalid.",
pp->name);
continue; continue;
} }
sc = gp->softc; sc = gp->softc;
if (g_union_get_writelock(sc) != 0) { if (g_union_get_writelock(sc) != 0) {
gctl_msg(req, "Commit already in progress for " gctl_msg(req, EINVAL, "Commit already in progress for "
"provider %s.", pp->name); "provider %s.", pp->name);
continue; continue;
} }
@ -661,10 +666,10 @@ g_union_ctl_commit(struct gctl_req *req, struct g_class *mp, bool verbose)
*/ */
if ((*force == false && pp->acr > 0) || pp->acw > 0 || if ((*force == false && pp->acr > 0) || pp->acw > 0 ||
pp->ace > 0) { pp->ace > 0) {
gctl_msg(req, "Unable to get exclusive access for " gctl_msg(req, EPERM, "Unable to get exclusive access "
"writing of %s.\n\tNote that %s cannot be mounted " "for writing of %s.\n\tNote that %s cannot be "
"or otherwise\n\topen during a commit unless the " "mounted or otherwise\n\topen during a commit "
"-f flag is used.", pp->name, pp->name); "unless the -f flag is used.", pp->name, pp->name);
g_union_rel_writelock(sc); g_union_rel_writelock(sc);
continue; continue;
} }
@ -675,7 +680,7 @@ g_union_ctl_commit(struct gctl_req *req, struct g_class *mp, bool verbose)
if ((*force == false && lowerpp->acr > lowercp->acr) || if ((*force == false && lowerpp->acr > lowercp->acr) ||
lowerpp->acw > lowercp->acw || lowerpp->acw > lowercp->acw ||
lowerpp->ace > lowercp->ace) { lowerpp->ace > lowercp->ace) {
gctl_msg(req, "provider %s is unable to get " gctl_msg(req, EPERM, "provider %s is unable to get "
"exclusive access to %s\n\tfor writing. Note that " "exclusive access to %s\n\tfor writing. Note that "
"%s cannot be mounted or otherwise open\n\tduring " "%s cannot be mounted or otherwise open\n\tduring "
"a commit unless the -f flag is used.", pp->name, "a commit unless the -f flag is used.", pp->name,
@ -684,8 +689,8 @@ g_union_ctl_commit(struct gctl_req *req, struct g_class *mp, bool verbose)
continue; continue;
} }
if ((error = g_access(lowercp, 0, 1, 0)) != 0) { if ((error = g_access(lowercp, 0, 1, 0)) != 0) {
gctl_msg(req, "Error %d: provider %s is unable to " gctl_msg(req, error, "Error %d: provider %s is unable "
"access %s for writing.", error, pp->name, "to access %s for writing.", error, pp->name,
lowerpp->name); lowerpp->name);
g_union_rel_writelock(sc); g_union_rel_writelock(sc);
continue; continue;
@ -715,18 +720,18 @@ g_union_ctl_commit(struct gctl_req *req, struct g_class *mp, bool verbose)
bp->bio_cmd = BIO_READ; bp->bio_cmd = BIO_READ;
g_io_request(bp, sc->sc_uppercp); g_io_request(bp, sc->sc_uppercp);
if ((error = biowait(bp, "rdunion")) != 0) { if ((error = biowait(bp, "rdunion")) != 0) {
gctl_msg(req, "Commit read error %d " gctl_msg(req, error, "Commit read "
"in provider %s, commit aborted.", "error %d in provider %s, commit "
error, pp->name); "aborted.", error, pp->name);
goto cleanup; goto cleanup;
} }
bp->bio_flags &= ~BIO_DONE; bp->bio_flags &= ~BIO_DONE;
bp->bio_cmd = BIO_WRITE; bp->bio_cmd = BIO_WRITE;
g_io_request(bp, lowercp); g_io_request(bp, lowercp);
if ((error = biowait(bp, "wtunion")) != 0) { if ((error = biowait(bp, "wtunion")) != 0) {
gctl_msg(req, "Commit write error %d " gctl_msg(req, error, "Commit write "
"in provider %s, commit aborted.", "error %d in provider %s, commit "
error, pp->name); "aborted.", error, pp->name);
goto cleanup; goto cleanup;
} }
bp->bio_flags &= ~BIO_DONE; bp->bio_flags &= ~BIO_DONE;
@ -748,7 +753,7 @@ g_union_ctl_commit(struct gctl_req *req, struct g_class *mp, bool verbose)
} }
g_union_rel_writelock(sc); g_union_rel_writelock(sc);
if (error == 0 && verbose) if (error == 0 && verbose)
gctl_msg(req, "Device %s has been committed.", gctl_msg(req, 0, "Device %s has been committed.",
pp->name); pp->name);
G_UNION_DEBUG(1, "Device %s has been committed.", pp->name); G_UNION_DEBUG(1, "Device %s has been committed.", pp->name);
} }
@ -1315,13 +1320,13 @@ g_union_destroy(struct gctl_req *req, struct g_geom *gp, bool force)
(pp != NULL && (pp->acr != 0 || pp->acw != 0 || pp->ace != 0))) { (pp != NULL && (pp->acr != 0 || pp->acw != 0 || pp->ace != 0))) {
if (force) { if (force) {
if (req != NULL) if (req != NULL)
gctl_msg(req, "Device %s is still in use, " gctl_msg(req, 0, "Device %s is still in use, "
"so is being forcibly removed.", gp->name); "so is being forcibly removed.", gp->name);
G_UNION_DEBUG(1, "Device %s is still in use, so " G_UNION_DEBUG(1, "Device %s is still in use, so "
"is being forcibly removed.", gp->name); "is being forcibly removed.", gp->name);
} else { } else {
if (req != NULL) if (req != NULL)
gctl_msg(req, "Device %s is still open " gctl_msg(req, EBUSY, "Device %s is still open "
"(r=%d w=%d e=%d).", gp->name, pp->acr, "(r=%d w=%d e=%d).", gp->name, pp->acr,
pp->acw, pp->ace); pp->acw, pp->ace);
G_UNION_DEBUG(1, "Device %s is still open " G_UNION_DEBUG(1, "Device %s is still open "
@ -1331,7 +1336,7 @@ g_union_destroy(struct gctl_req *req, struct g_geom *gp, bool force)
} }
} else { } else {
if (req != NULL) if (req != NULL)
gctl_msg(req, "Device %s removed.", gp->name); gctl_msg(req, 0, "Device %s removed.", gp->name);
G_UNION_DEBUG(1, "Device %s removed.", gp->name); G_UNION_DEBUG(1, "Device %s removed.", gp->name);
} }
/* Close consumers */ /* Close consumers */