[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z9fuFphLv5pM7AC_@google.com>
Date: Mon, 17 Mar 2025 09:40:38 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>,
Nathan Chancellor <nathan@...nel.org>
Subject: Re: [PATCH 09/13] objtool: Add --output option
On Fri, Mar 14, 2025 at 12:29:07PM -0700, Josh Poimboeuf wrote:
> Add option to allow writing the changed binary to a separate file rather
> than changing it in place.
>
> Libelf makes this suprisingly hard, so take the easy way out and just
> copy the file before editing it.
>
> Also steal the -o short option from --orc. Nobody will notice ;-)
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
> ---
> tools/objtool/builtin-check.c | 98 ++++++++++++++++++++-----
> tools/objtool/elf.c | 3 -
> tools/objtool/include/objtool/builtin.h | 1 +
> tools/objtool/objtool.c | 15 ++--
> tools/objtool/orc_dump.c | 7 +-
> 5 files changed, 89 insertions(+), 35 deletions(-)
>
> diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
> index 79843512a51b..3de3afa0a19c 100644
> --- a/tools/objtool/builtin-check.c
> +++ b/tools/objtool/builtin-check.c
> @@ -6,6 +6,10 @@
> #include <subcmd/parse-options.h>
> #include <string.h>
> #include <stdlib.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +#include <sys/sendfile.h>
> #include <objtool/builtin.h>
> #include <objtool/objtool.h>
>
> @@ -14,6 +18,8 @@
> "error: objtool: " format "\n", \
> ##__VA_ARGS__)
>
> +const char *objname;
> +
> struct opts opts;
>
> static const char * const check_usage[] = {
> @@ -71,7 +77,7 @@ static const struct option check_options[] = {
> OPT_BOOLEAN('i', "ibt", &opts.ibt, "validate and annotate IBT"),
> OPT_BOOLEAN('m', "mcount", &opts.mcount, "annotate mcount/fentry calls for ftrace"),
> OPT_BOOLEAN('n', "noinstr", &opts.noinstr, "validate noinstr rules"),
> - OPT_BOOLEAN('o', "orc", &opts.orc, "generate ORC metadata"),
> + OPT_BOOLEAN(0, "orc", &opts.orc, "generate ORC metadata"),
> OPT_BOOLEAN('r', "retpoline", &opts.retpoline, "validate and annotate retpoline usage"),
> OPT_BOOLEAN(0, "rethunk", &opts.rethunk, "validate and annotate rethunk usage"),
> OPT_BOOLEAN(0, "unret", &opts.unret, "validate entry unret placement"),
> @@ -84,15 +90,16 @@ static const struct option check_options[] = {
> OPT_CALLBACK_OPTARG(0, "dump", NULL, NULL, "orc", "dump metadata", parse_dump),
>
> OPT_GROUP("Options:"),
> - OPT_BOOLEAN(0, "backtrace", &opts.backtrace, "unwind on error"),
> - OPT_BOOLEAN(0, "backup", &opts.backup, "create .orig files before modification"),
> - OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"),
> - OPT_BOOLEAN(0, "link", &opts.link, "object is a linked object"),
> - OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
> - OPT_BOOLEAN(0, "mnop", &opts.mnop, "nop out mcount call sites"),
> - OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
> - OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
> - OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
> + OPT_BOOLEAN(0, "backtrace", &opts.backtrace, "unwind on error"),
> + OPT_BOOLEAN(0, "backup", &opts.backup, "create .orig files before modification"),
> + OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"),
> + OPT_BOOLEAN(0, "link", &opts.link, "object is a linked object"),
> + OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
> + OPT_BOOLEAN(0, "mnop", &opts.mnop, "nop out mcount call sites"),
> + OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
> + OPT_STRING('o', "output", &opts.output, "file", "output file name"),
I think the docstring should say that it edits in place if the option
isn't provided.
> + OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
> + OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
> OPT_BOOLEAN('v', "verbose", &opts.verbose, "verbose warnings"),
>
> OPT_END(),
> @@ -178,24 +185,75 @@ static bool opts_valid(void)
> return false;
> }
>
> +static int copy_file(const char *src, const char *dst)
> +{
> + size_t to_copy, copied;
> + int dst_fd, src_fd;
> + struct stat stat;
> + off_t offset = 0;
> +
> + src_fd = open(src, O_RDONLY);
> + if (src_fd == -1) {
> + ERROR("can't open '%s' for reading", src);
> + return 1;
> + }
> +
> + dst_fd = open(dst, O_WRONLY | O_CREAT | O_TRUNC);
> + if (dst_fd == -1) {
> + ERROR("can't open '%s' for writing", dst);
Please include strerror(errno) here and above.
> + return 1;
The annoying pedantry demon on my shoulder is compelling me to point
out that this and the following error returns leak file descriptors.
To be honest, I don't care in the slightest whether or not this "bug"
gets "fixed" but at least I quelled my pedantry demon by pointing it
out.
Powered by blists - more mailing lists