LU-12757 utils: avoid newline inside error message

When calling llapi_error() the format string should not end in a
newline, since the error string is appended to the output with
its own newline.

Fix several callers to not supply their own newline, and callers
that duplicate the error string in the error message itself.

In the case that there are callers that *do* include a newline,
handle this gracefully to avoid splitting the error across lines.

Test-Parameters: trivial
Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
Change-Id: Ie8f7206d82faccb3b33e2fc62b00f5226b3ebbe5
Reviewed-on: https://review.whamcloud.com/36176
Reviewed-by: Ben Evans <bevans@cray.com>
Tested-by: jenkins <devops@whamcloud.com>
Tested-by: Maloo <maloo@whamcloud.com>
Reviewed-by: Jian Yu <yujian@whamcloud.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lustre/utils/liblustreapi.c b/lustre/utils/liblustreapi.c
index 726b520..77998f4 100644 (file)
--- a/lustre/utils/liblustreapi.c
+++ b/lustre/utils/liblustreapi.c
@@ -126,16 +126,35 @@ void llapi_clear_command_name(void)
 static void error_callback_default(enum llapi_message_level level, int err,
                                   const char *fmt, va_list ap)
 {
+       bool has_nl = strchr(fmt, '\n') != NULL;
+
        if (liblustreapi_cmd != NULL)
                fprintf(stderr, "%s %s: ", program_invocation_short_name,
                        liblustreapi_cmd);
        else
                fprintf(stderr, "%s: ", program_invocation_short_name);
-       vfprintf(stderr, fmt, ap);
-       if (level & LLAPI_MSG_NO_ERRNO)
-               fprintf(stderr, "\n");
-       else
+
+
+       if (level & LLAPI_MSG_NO_ERRNO) {
+               vfprintf(stderr, fmt, ap);
+               if (!has_nl)
+                       fprintf(stderr, "\n");
+       } else {
+               char *newfmt;
+
+               /* Remove trailing linefeed so error string can be appended.
+                * @fmt is a const string, so we can't modify it directly.
+                */
+               if (has_nl && (newfmt = strdup(fmt)))
+                       *strrchr(newfmt, '\n') = '\0';
+               else
+                       newfmt = (char *)fmt;
+
+               vfprintf(stderr, newfmt, ap);
+               if (newfmt != fmt)
+                       free(newfmt);
                fprintf(stderr, ": %s (%d)\n", strerror(err), err);
+       }
 }
 
 static void info_callback_default(enum llapi_message_level level, int err,
@@ -302,7 +321,7 @@ int llapi_ioctl_pack(struct obd_ioctl_data *data, char **pbuf, int max_len)
 
        if (*pbuf != NULL && data->ioc_len > max_len) {
                llapi_error(LLAPI_MSG_ERROR, -EINVAL,
-                           "pbuf = %p, ioc_len = %u, max_len = %d\n",
+                           "pbuf = %p, ioc_len = %u, max_len = %d",
                            *pbuf, data->ioc_len, max_len);
                return -EINVAL;
        }
@@ -1124,10 +1143,10 @@ int llapi_dir_create_foreign(const char *name, mode_t mode, __u32 type,
        if (len > XATTR_SIZE_MAX - offsetof(struct lmv_foreign_md, lfm_value) ||
            len <= 0) {
                rc = -EINVAL;
-               llapi_error(LLAPI_MSG_ERROR, rc, "invalid LOV EA length %zu "
-                       "(must be 0 < len < %zu)\n", len,
-                       XATTR_SIZE_MAX -
-                       offsetof(struct lmv_foreign_md, lfm_value));
+               llapi_error(LLAPI_MSG_ERROR, rc,
+                           "invalid LOV EA length %zu (must be 0 < len < %zu)",
+                           len, XATTR_SIZE_MAX -
+                           offsetof(struct lmv_foreign_md, lfm_value));
                return rc;
        }
        lfm_size = len + offsetof(struct lmv_foreign_md, lfm_value);
@@ -1235,13 +1254,10 @@ int llapi_direntry_remove(char *dname)
                goto out;
        }
 
-       if (ioctl(fd, LL_IOC_REMOVE_ENTRY, filename)) {
-               char *errmsg = strerror(errno);
-               llapi_err_noerrno(LLAPI_MSG_ERROR,
-                                 "error on ioctl %#jx for '%s' (%d): %s",
-                                 (uintmax_t)LL_IOC_LMV_SETSTRIPE, filename,
-                                 fd, errmsg);
-       }
+       if (ioctl(fd, LL_IOC_REMOVE_ENTRY, filename))
+               llapi_error(LLAPI_MSG_ERROR, errno,
+                           "error on ioctl %#lx for '%s' (%d)",
+                           (long)LL_IOC_LMV_SETSTRIPE, filename, fd);
 out:
        free(dirpath);
        free(namepath);
@@ -1339,8 +1355,7 @@ int get_root_path(int want, char *fsname, int *outfd, char *path, int index)
                        if (fd < 0) {
                                rc = -errno;
                                llapi_error(LLAPI_MSG_ERROR, rc,
-                                           "cannot open '%s': %s", mntdir,
-                                           strerror(-rc));
+                                           "cannot open '%s'", mntdir);
 
                        } else {
                                *outfd = fd;
@@ -4118,12 +4133,9 @@ static int print_failed_tgt(struct find_param *param, char *path, int type)
        ret = llapi_obd_statfs(path, type,
                               param->fp_obd_index, &stat_buf,
                               &uuid_buf);
-       if (ret) {
-               llapi_printf(LLAPI_MSG_NORMAL,
-                            "obd_uuid: %s failed %s ",
-                            param->fp_obd_uuid->uuid,
-                            strerror(errno));
-       }
+       if (ret)
+               llapi_error(LLAPI_MSG_NORMAL, ret, "obd_uuid: %s failed",
+                            param->fp_obd_uuid->uuid);
 
        return ret;
 }
@@ -4996,14 +5008,13 @@ migrate:
                } else if (errno == EALREADY) {
                        if (param->fp_verbose & VERBOSE_DETAIL)
                                llapi_printf(LLAPI_MSG_NORMAL,
-                                            "%s was migrated to MDT%d already\n",
+                                            "%s migrated to MDT%d already\n",
                                             path, lmu->lum_stripe_offset);
                        ret = 0;
                } else {
                        ret = -errno;
-                       llapi_error(LLAPI_MSG_ERROR, ret,
-                                   "%s migrate failed: %s (%d)\n",
-                                   path, strerror(-ret), ret);
+                       llapi_error(LLAPI_MSG_ERROR, ret, "%s migrate failed",
+                                   path);
                        goto out;
                }
        } else if (param->fp_verbose & VERBOSE_DETAIL) {
@@ -5080,7 +5091,8 @@ int llapi_mv(char *path, struct find_param *param)
 
        if (!printed) {
                llapi_error(LLAPI_MSG_ERROR, -ESTALE,
-                           "llapi_mv() is deprecated, use llapi_migrate_mdt()\n");
+                         "%s() is deprecated, use llapi_migrate_mdt() instead",
+                         __func__);
                printed = true;
        }
 #endif

diff --git a/lustre/utils/liblustreapi_layout.c b/lustre/utils/liblustreapi_layout.c
index e47b303..e6ad5fd 100644 (file)
--- a/lustre/utils/liblustreapi_layout.c
+++ b/lustre/utils/liblustreapi_layout.c
@@ -2935,7 +2935,7 @@ int llapi_mirror_resync_many(int fd, struct llapi_layout *layout,
                                 */
                                comp_array[i].lrc_synced = true;
                                llapi_error(LLAPI_MSG_ERROR, written,
-                                           "component %u not synced\n",
+                                           "component %u not synced",
                                            comp_array[i].lrc_id);
                                if (rc2 == 0)
                                        rc2 = (int)written;

diff --git a/lustre/utils/liblustreapi_pcc.c b/lustre/utils/liblustreapi_pcc.c
index 398239d..f538e02 100644 (file)
--- a/lustre/utils/liblustreapi_pcc.c
+++ b/lustre/utils/liblustreapi_pcc.c
@@ -344,7 +344,7 @@ int llapi_pccdev_set(const char *mntpath, const char *cmd)
        rc = llapi_getname(mntpath, buf, sizeof(buf));
        if (rc < 0) {
                llapi_error(LLAPI_MSG_ERROR, rc,
-                           "cannot get name for '%s'\n", mntpath);
+                           "cannot get name for '%s'", mntpath);
                return rc;
        }
 
@@ -365,12 +365,12 @@ int llapi_pccdev_set(const char *mntpath, const char *cmd)
                rc = errno;
                if (errno != EIO)
                        llapi_error(LLAPI_MSG_ERROR, rc,
-                                   "error: setting llite.%s.pcc=\"%s\"\n",
+                                   "error: setting llite.%s.pcc='%s'",
                                    buf, cmd);
        } else if (count < strlen(cmd)) { /* Truncate case */
                rc = -EINVAL;
                llapi_error(LLAPI_MSG_ERROR, rc,
-                           "setting llite.%s.pcc=\"%s\": wrote only %zd\n",
+                           "setting llite.%s.pcc='%s': wrote only %zd",
                            buf, cmd, count);
        }
        close(fd);
@@ -394,7 +394,7 @@ int llapi_pccdev_get(const char *mntpath)
        rc = llapi_getname(mntpath, pathbuf, sizeof(pathbuf));
        if (rc < 0) {
                llapi_error(LLAPI_MSG_ERROR, rc,
-                           "cannot get name for '%s'\n", mntpath);
+                           "cannot get name for '%s'", mntpath);
                return rc;
        }
 
@@ -407,7 +407,7 @@ int llapi_pccdev_get(const char *mntpath)
        if (fd < 0) {
                rc = -errno;
                llapi_error(LLAPI_MSG_ERROR, rc,
-                           "error: pccdev_get: opening '%s'\n",
+                           "error: pccdev_get: opening '%s'",
                            path.gl_pathv[0]);
                goto out_free_param;
        }
@@ -416,7 +416,7 @@ int llapi_pccdev_get(const char *mntpath)
        if (buf == NULL) {
                rc = -ENOMEM;
                llapi_error(LLAPI_MSG_ERROR, rc,
-                           "error: pccdev_get: allocating '%s' buffer\n",
+                           "error: pccdev_get: allocating '%s' buffer",
                            path.gl_pathv[0]);
                goto out_close;
        }
@@ -430,8 +430,7 @@ int llapi_pccdev_get(const char *mntpath)
                        rc = -errno;
                        if (errno != EIO) {
                                llapi_error(LLAPI_MSG_ERROR, rc,
-                                           "error: pccdev_get: "
-                                           "reading failed\n");
+                                          "error: pccdev_get: reading failed");
                        }
                        break;
                }
@@ -439,7 +438,7 @@ int llapi_pccdev_get(const char *mntpath)
                if (fwrite(buf, 1, count, stdout) != count) {
                        rc = -errno;
                        llapi_error(LLAPI_MSG_ERROR, rc,
-                                   "error: get_param: write to stdout\n");
+                                   "error: get_param: write to stdout");
                        break;
                }
        }

diff --git a/lustre/utils/llog_reader.c b/lustre/utils/llog_reader.c
index f498b54..01e59de 100644 (file)
--- a/lustre/utils/llog_reader.c
+++ b/lustre/utils/llog_reader.c
@@ -185,7 +185,8 @@ int main(int argc, char **argv)
        is_ext = is_fstype_ext(fd);
        if (is_ext < 0) {
                rc = is_ext;
-               printf("Unable to determine type of filesystem containing %s\n",
+               llapi_error(LLAPI_MSG_ERROR, -rc,
+                           "Unable to determine filesystem type for %s",
                       argv[1]);
                goto out_fd;
        }
@@ -233,8 +234,7 @@ int llog_pack_buffer(int fd, struct llog_log_hdr **llog,
        file_size = st.st_size;
        if (file_size < sizeof(**llog)) {
                llapi_error(LLAPI_MSG_ERROR, rc,
-                           "File too small for llog header: "
-                           "need %zd, size %lld\n",
+                           "File too small for llog header: want=%zd got=%lld",
                            sizeof(**llog), file_size);
                rc = -EIO;
                goto out;

diff --git a/lustre/utils/llsom_sync.c b/lustre/utils/llsom_sync.c
index 73260ac..9d1069c 100644 (file)
--- a/lustre/utils/llsom_sync.c
+++ b/lustre/utils/llsom_sync.c
@@ -473,7 +473,7 @@ int main(int argc, char **argv)
                default:
                        rc = -EINVAL;
                        llapi_error(LLAPI_MSG_ERROR, rc,
-                                   "%s: unknown option '-%c'\n",
+                                   "%s: unknown option '%c'",
                                    argv[0], optopt);
                        return rc;
                case 'u':
@@ -582,7 +582,7 @@ int main(int argc, char **argv)
                                           opt.o_mdtname, 0);
                if (rc) {
                        llapi_error(LLAPI_MSG_ERROR, rc,
-                                   "unable to open changelog of MDT [%s]\n",
+                                   "unable to open changelog of MDT '%s'",
                                    opt.o_mdtname);
                        return rc;
                }
@@ -628,7 +628,7 @@ int main(int argc, char **argv)
                rc = llapi_changelog_fini(&chglog_hdlr);
                if (rc) {
                        llapi_error(LLAPI_MSG_ERROR, rc,
-                                   "unable to close changelog of MDT [%s]",
+                                   "unable to close changelog of MDT '%s'",
                                    opt.o_mdtname);
                        ret = rc;
                        return rc;