On 9/25/19 4:35 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Refactor all the places in the code where we try to render an inode
> number as a prefix for some sort of status message. This will help make
> message prefixes more consistent, which should help users to locate
> broken metadata.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> scrub/common.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> scrub/common.h | 6 ++++++
> scrub/inodes.c | 4 ++--
> scrub/phase3.c | 8 ++------
> scrub/phase5.c | 8 ++------
> scrub/phase6.c | 6 +++---
> scrub/scrub.c | 17 +++++++++--------
> 7 files changed, 71 insertions(+), 25 deletions(-)
>
>
> diff --git a/scrub/common.c b/scrub/common.c
> index 7db47044..a814f568 100644
> --- a/scrub/common.c
> +++ b/scrub/common.c
> @@ -354,3 +354,50 @@ within_range(
>
> return true;
> }
> +
> +/*
> + * Render an inode number as both the raw inode number and as an AG number
> + * and AG inode pair. This is intended for use with status message reporting.
> + * If @format is not NULL, it should provide any desired leading whitespace.
> + *
> + * For example, "inode 287232352 (13/352) <suffix>: <status message>"
> + */
This comment is a bit odd, the <suffix>: part seems to indicate something special
but presumably it's just part of the provided format ... although the function
is named scrub_render_ino_suffix so... ???
Also: can we do away with requiring a leading space in the format? That seems
a little bit error prone and funky.
> +int
> +scrub_render_ino_suffix(
> + const struct scrub_ctx *ctx,
> + char *buf,
> + size_t buflen,
> + uint64_t ino,
> + uint32_t gen,
> + const char *format,
> + ...)
> +{
> + va_list args;
> + uint32_t agno;
> + uint32_t agino;
> + int ret;
> +
> + agno = cvt_ino_to_agno(&ctx->mnt, ino);
> + agino = cvt_ino_to_agino(&ctx->mnt, ino);
> + ret = snprintf(buf, buflen, _("inode %"PRIu64" (%"PRIu32"/%"PRIu32")"),
> + ino, agno, agino);
> + if (ret < 0 || ret >= buflen || format == NULL)
> + return ret;
> +
> + va_start(args, format);
> + ret += vsnprintf(buf + ret, buflen - ret, format, args);
> + va_end(args);
> + return ret;
> +}
> +
> +/* Render an inode number for message reporting with no suffix. */
> +int
> +scrub_render_ino(
> + const struct scrub_ctx *ctx,
> + char *buf,
> + size_t buflen,
> + uint64_t ino,
> + uint32_t gen)
> +{
> + return scrub_render_ino_suffix(ctx, buf, buflen, ino, gen, NULL);
> +}
Could we call these scrub_render_ino / scrub_render_ino_message or
something? That seems more clear to me.
Actually, why not just do away w/ the wrapper altogether and just call
it with NULL if you don't want more verbage. That seems clear enough.
So maybe just:
/*
* Render an inode number into a buffer in a format suitable for use as a
* prefix for log messages. The buffer will be filled with:
* "inode <inode number> (<ag number>/<ag inode number>)"
* If the @format argument is non-NULL, it will be rendered into the buffer
* immediately afterwards.
*/
int
scrub_render_ino_descr(...
because that's how it's generally used, right?
-Eric
