From nobody Sat Jul 16 17:27:23 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4LlZt36VR0z4X3gr; Sat, 16 Jul 2022 17:27:23 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4LlZt36HLbz3qhM; Sat, 16 Jul 2022 17:27:23 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1657992443; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=jJP50hUgJ+TbzHR+pUe9KQcOviTRp2NUy+mdF5j/QbY=; b=n4Q0X1dZBNlssRY8jVOsxlG8NPwiLju3pR5H1bxpFAdMNVEt9OJk42cXvXb4q+HBpqTAt6 ICdvR2V8kRaZn6TKXcmGi6JTtiq5n750CMiHQk6r9IK0hlzarxyZJBJgFocJ8ujBzE5kFP xxZR050F2eAlvZxzw8PHjRz/c8zXmcJW1Nl4nZe77tHFt1W75XyVce5QKGlh5wAgPA2Yfy WA4bAh2vKwTBdZnS/JIKHUkFJjpWEBwyuRatq5ul+kBHKCzC6k1KlyIVhtOonegXAe0RX8 F9uDsB/nNwFp/fqZg3RGoHD7+ubWepPUcXYF5TzN+V+gjNtQKJDxjjvZsgU3Eg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4LlZt35KckzTHG; Sat, 16 Jul 2022 17:27:23 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 26GHRN9L055356; Sat, 16 Jul 2022 17:27:23 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 26GHRNIc055355; Sat, 16 Jul 2022 17:27:23 GMT (envelope-from git) Date: Sat, 16 Jul 2022 17:27:23 GMT Message-Id: <202207161727.26GHRNIc055355@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Kirk McKusick Subject: git: 90e29718cffc - main - Clarify when GEOM utilities exit with success or failure. List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: mckusick X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 90e29718cffcec987769ccbe39308357202c46d5 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1657992443; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=jJP50hUgJ+TbzHR+pUe9KQcOviTRp2NUy+mdF5j/QbY=; b=m+6KeXqihLt1FPjqxVZS92knaHme/Xq8ppvVAGJ2jPbOWxrPi2fZRMYLyXV6h37NUA25Wm acdyfAziIeh8aDHEhkCqULeslOBLl+oWjQBB3VFqsmMd7L+O12qCnzMuho74dBjB5zdJPb fOggvv5FpM6zxHLh5TWzUq1SUE2CGtxeStrTnmDqskOaYwbb51FBXH/J8vyup7D1hwD7wX fNpstZCdcn93sjOZ1Qz6RwjlorPCXsIQyF7AKG+lf2XbiLEOHDAPfUtzSRc3WQiDS4ivUC /v/h2BsLnFS05NS6h1q18wIrgF/gtHjpvsrA74xhpBjTUF+0dUO4arKoyJ9Ckg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1657992443; a=rsa-sha256; cv=none; b=s8ZCR6vr1xhvqi37diPBBy3H9/YmsdFf31QOQHU2rm2SdwNtc324c/C97KI71ZOvobvJnN vLDd6WjFs0zthef3dNDcQSpMuzzxzXEsMTUAV9gKJpYz+AY6KufeTM71nUlk/rI4X5C1lC kSd+mfuir7vdXQ25uWtjtASqWfktj+gklyY5COiKuUWh1KFyhD3Pvu6zUiVHJExcLKgz0o qvIV1cxKHmE/iU3ofFay5qBU7fSV/0l4uBDyEeBhKWoIvnFOBWpir3NlXzzwLFnfO8mrdj MrYkV0fAzdwTlHR7waIS2R/JhG/Aym6j9iAXly4Um2nERYetIq9riSM2Pnw/Jg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by mckusick: URL: https://cgit.FreeBSD.org/src/commit/?id=90e29718cffcec987769ccbe39308357202c46d5 commit 90e29718cffcec987769ccbe39308357202c46d5 Author: Kirk McKusick AuthorDate: 2022-07-16 17:25:22 +0000 Commit: Kirk McKusick 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 */