git: 90e29718cffc - main - Clarify when GEOM utilities exit with success or failure.
Date: Sat, 16 Jul 2022 17:27:23 UTC
The branch main has been updated by mckusick:
URL: https://cgit.FreeBSD.org/src/commit/?id=90e29718cffcec987769ccbe39308357202c46d5
commit 90e29718cffcec987769ccbe39308357202c46d5
Author: Kirk McKusick <mckusick@FreeBSD.org>
AuthorDate: 2022-07-16 17:25:22 +0000
Commit: Kirk McKusick <mckusick@FreeBSD.org>
CommitDate: 2022-07-16 17:26:51 +0000
Clarify when GEOM utilities exit with success or failure.
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
---
lib/libgeom/geom_ctl.c | 6 ++--
sbin/geom/core/geom.c | 5 +++-
sys/geom/geom.h | 2 +-
sys/geom/geom_ctl.c | 10 ++++++-
sys/geom/union/g_union.c | 75 ++++++++++++++++++++++++++----------------------
5 files changed, 57 insertions(+), 41 deletions(-)
diff --git a/lib/libgeom/geom_ctl.c b/lib/libgeom/geom_ctl.c
index b60c4c297257..b1d738333b3f 100644
--- a/lib/libgeom/geom_ctl.c
+++ b/lib/libgeom/geom_ctl.c
@@ -194,7 +194,7 @@ gctl_rw_param(struct gctl_req *req, const char *name, int len, void *value)
const char *
gctl_issue(struct gctl_req *req)
{
- int fd, error;
+ int fd;
if (req == NULL)
return ("NULL request pointer");
@@ -212,11 +212,11 @@ gctl_issue(struct gctl_req *req)
fd = open(_PATH_DEV PATH_GEOM_CTL, O_RDONLY);
if (fd < 0)
return(strerror(errno));
- error = ioctl(fd, GEOM_CTL, req);
+ req->nerror = ioctl(fd, GEOM_CTL, req);
close(fd);
if (req->error[0] != '\0')
return (req->error);
- if (error != 0)
+ if (req->nerror == -1)
return(strerror(errno));
return (NULL);
}
diff --git a/sbin/geom/core/geom.c b/sbin/geom/core/geom.c
index 9b43910b88f9..535d7d9f6f21 100644
--- a/sbin/geom/core/geom.c
+++ b/sbin/geom/core/geom.c
@@ -503,7 +503,10 @@ run_command(int argc, char *argv[])
}
if (errstr != NULL && errstr[0] != '\0') {
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);
exit(EXIT_FAILURE);
}
diff --git a/sys/geom/geom.h b/sys/geom/geom.h
index 70d21e346c9b..a9990f669863 100644
--- a/sys/geom/geom.h
+++ b/sys/geom/geom.h
@@ -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_opt(struct gctl_req *req, const char *param, int len);
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);
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);
diff --git a/sys/geom/geom_ctl.c b/sys/geom/geom_ctl.c
index 0f1dce934b63..132f30070b3c 100644
--- a/sys/geom/geom_ctl.c
+++ b/sys/geom/geom_ctl.c
@@ -117,9 +117,15 @@ gctl_error(struct gctl_req *req, const char *fmt, ...)
* the application must either call gctl_post_messages() or
* call gctl_error() to cause the messages to be reported to
* 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
-gctl_msg(struct gctl_req *req, const char *fmt, ...)
+gctl_msg(struct gctl_req *req, int errno, const char *fmt, ...)
{
va_list ap;
@@ -131,6 +137,8 @@ gctl_msg(struct gctl_req *req, const char *fmt, ...)
#endif
return;
}
+ if (req->nerror == 0)
+ req->nerror = errno;
/* Put second and later messages indented on a new line */
if (sbuf_len(req->serror) > 0)
sbuf_cat(req->serror, "\n\t");
diff --git a/sys/geom/union/g_union.c b/sys/geom/union/g_union.c
index ddc0acf52b78..dee541064ced 100644
--- a/sys/geom/union/g_union.c
+++ b/sys/geom/union/g_union.c
@@ -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) *
sizeof(uint64_t) + roundup(sc->sc_root_size, BITS_PER_ENTRY);
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);
+ gctl_post_messages(req);
G_UNION_DEBUG(1, "Device %s created with memory map size %jd.",
gp->name, (intmax_t)sc->sc_writemap_memory);
return;
@@ -373,7 +374,8 @@ g_union_fetcharg(struct gctl_req *req, const char *name)
return (0);
if (*val >= 0)
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);
}
@@ -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);
name = gctl_get_asciiparam(req, param);
if (name == NULL) {
- gctl_msg(req, "No '%s' argument.", param);
+ gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue;
}
if (strncmp(name, _PATH_DEV, strlen(_PATH_DEV)) == 0)
name += strlen(_PATH_DEV);
gp = g_union_find_geom(mp, name);
if (gp == NULL) {
- gctl_msg(req, "Device %s is invalid.", name);
+ gctl_msg(req, EINVAL, "Device %s is invalid.", name);
continue;
}
error = g_union_destroy(verbose ? req : NULL, gp, *force);
if (error != 0)
- gctl_msg(req, "Error %d: cannot destroy device %s.",
- error, gp->name);
+ gctl_msg(req, error, "Error %d: "
+ "cannot destroy device %s.", error, gp->name);
}
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);
pp = gctl_get_provider(req, param);
if (pp == NULL) {
- gctl_msg(req, "No '%s' argument.", param);
+ gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue;
}
gp = pp->geom;
if (gp->class != mp) {
- gctl_msg(req, "Provider %s is invalid.",
+ gctl_msg(req, EINVAL, "Provider %s is invalid.",
pp->name);
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_wrotebytes = 0;
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);
}
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);
pp = gctl_get_provider(req, param);
if (pp == NULL) {
- gctl_msg(req, "No '%s' argument.", param);
+ gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue;
}
gp = pp->geom;
if (gp->class != mp) {
- gctl_msg(req, "Provider %s is invalid.", pp->name);
+ gctl_msg(req, EINVAL, "Provider %s is invalid.",
+ pp->name);
continue;
}
sc = gp->softc;
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);
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.
*/
if (pp->acr > 0 || pp->acw > 0 || pp->ace > 0) {
- gctl_msg(req, "Unable to get exclusive access for "
- "reverting of %s;\n\t%s cannot be mounted or "
+ gctl_msg(req, EPERM, "Unable to get exclusive access "
+ "for reverting of %s;\n\t%s cannot be mounted or "
"otherwise open during a revert.",
pp->name, pp->name);
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_rel_writelock(sc);
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);
}
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);
pp = gctl_get_provider(req, param);
if (pp == NULL) {
- gctl_msg(req, "No '%s' argument.", param);
+ gctl_msg(req, EINVAL, "No '%s' argument.", param);
continue;
}
gp = pp->geom;
if (gp->class != mp) {
- gctl_msg(req, "Provider %s is invalid.", pp->name);
+ gctl_msg(req, EINVAL, "Provider %s is invalid.",
+ pp->name);
continue;
}
sc = gp->softc;
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);
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 ||
pp->ace > 0) {
- gctl_msg(req, "Unable to get exclusive access for "
- "writing of %s.\n\tNote that %s cannot be mounted "
- "or otherwise\n\topen during a commit unless the "
- "-f flag is used.", pp->name, pp->name);
+ gctl_msg(req, EPERM, "Unable to get exclusive access "
+ "for writing of %s.\n\tNote that %s cannot be "
+ "mounted or otherwise\n\topen during a commit "
+ "unless the -f flag is used.", pp->name, pp->name);
g_union_rel_writelock(sc);
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) ||
lowerpp->acw > lowercp->acw ||
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 "
"%s cannot be mounted or otherwise open\n\tduring "
"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;
}
if ((error = g_access(lowercp, 0, 1, 0)) != 0) {
- gctl_msg(req, "Error %d: provider %s is unable to "
- "access %s for writing.", error, pp->name,
+ gctl_msg(req, error, "Error %d: provider %s is unable "
+ "to access %s for writing.", error, pp->name,
lowerpp->name);
g_union_rel_writelock(sc);
continue;
@@ -715,18 +720,18 @@ g_union_ctl_commit(struct gctl_req *req, struct g_class *mp, bool verbose)
bp->bio_cmd = BIO_READ;
g_io_request(bp, sc->sc_uppercp);
if ((error = biowait(bp, "rdunion")) != 0) {
- gctl_msg(req, "Commit read error %d "
- "in provider %s, commit aborted.",
- error, pp->name);
+ gctl_msg(req, error, "Commit read "
+ "error %d in provider %s, commit "
+ "aborted.", error, pp->name);
goto cleanup;
}
bp->bio_flags &= ~BIO_DONE;
bp->bio_cmd = BIO_WRITE;
g_io_request(bp, lowercp);
if ((error = biowait(bp, "wtunion")) != 0) {
- gctl_msg(req, "Commit write error %d "
- "in provider %s, commit aborted.",
- error, pp->name);
+ gctl_msg(req, error, "Commit write "
+ "error %d in provider %s, commit "
+ "aborted.", error, pp->name);
goto cleanup;
}
bp->bio_flags &= ~BIO_DONE;
@@ -748,7 +753,7 @@ cleanup:
}
g_union_rel_writelock(sc);
if (error == 0 && verbose)
- gctl_msg(req, "Device %s has been committed.",
+ gctl_msg(req, 0, "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))) {
if (force) {
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);
G_UNION_DEBUG(1, "Device %s is still in use, so "
"is being forcibly removed.", gp->name);
} else {
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,
pp->acw, pp->ace);
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 {
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);
}
/* Close consumers */