From: David Matlack <dmatlack@google.com>
To: Vipin Sharma <vipinsh@google.com>
Cc: seanjc@google.com, pbonzini@redhat.com, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/4] KVM: selftests: Add atoi_paranoid to catch errors missed by atoi
Date: Wed, 21 Sep 2022 11:14:16 -0700 [thread overview]
Message-ID: <YytUeOhWEBJF6MMz@google.com> (raw)
In-Reply-To: <20220826184500.1940077-4-vipinsh@google.com>
On Fri, Aug 26, 2022 at 11:44:59AM -0700, Vipin Sharma wrote:
> atoi() doesn't detect errors. There is no way to know that a 0 return
> is correct conversion or due to an error.
>
> Introduce atoi_paranoid() to detect errors and provide correct
> conversion. Replace all atoi calls with atoi_paranoid.
Please use "()" after all function names.
>
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Suggested-by: David Matlack <dmatlack@google.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/aarch64/arch_timer.c | 8 ++++----
> tools/testing/selftests/kvm/aarch64/vgic_irq.c | 6 +++---
> .../selftests/kvm/access_tracking_perf_test.c | 2 +-
> tools/testing/selftests/kvm/demand_paging_test.c | 2 +-
> tools/testing/selftests/kvm/dirty_log_perf_test.c | 8 ++++----
> tools/testing/selftests/kvm/include/test_util.h | 2 ++
> tools/testing/selftests/kvm/kvm_page_table_test.c | 2 +-
> tools/testing/selftests/kvm/lib/test_util.c | 14 ++++++++++++++
> .../testing/selftests/kvm/max_guest_memory_test.c | 6 +++---
> .../kvm/memslot_modification_stress_test.c | 4 ++--
> tools/testing/selftests/kvm/memslot_perf_test.c | 10 +++++-----
> .../testing/selftests/kvm/set_memory_region_test.c | 2 +-
> .../selftests/kvm/x86_64/nx_huge_pages_test.c | 4 ++--
> 13 files changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> index 574eb73f0e90..251e7ff04883 100644
> --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> @@ -414,7 +414,7 @@ static bool parse_args(int argc, char *argv[])
> while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) {
> switch (opt) {
> case 'n':
> - test_args.nr_vcpus = atoi(optarg);
> + test_args.nr_vcpus = atoi_paranoid(optarg);
> if (test_args.nr_vcpus <= 0) {
> pr_info("Positive value needed for -n\n");
> goto err;
> @@ -425,21 +425,21 @@ static bool parse_args(int argc, char *argv[])
> }
> break;
> case 'i':
> - test_args.nr_iter = atoi(optarg);
> + test_args.nr_iter = atoi_paranoid(optarg);
> if (test_args.nr_iter <= 0) {
> pr_info("Positive value needed for -i\n");
> goto err;
> }
> break;
> case 'p':
> - test_args.timer_period_ms = atoi(optarg);
> + test_args.timer_period_ms = atoi_paranoid(optarg);
> if (test_args.timer_period_ms <= 0) {
> pr_info("Positive value needed for -p\n");
> goto err;
> }
> break;
> case 'm':
> - test_args.migration_freq_ms = atoi(optarg);
> + test_args.migration_freq_ms = atoi_paranoid(optarg);
> if (test_args.migration_freq_ms < 0) {
> pr_info("0 or positive value needed for -m\n");
> goto err;
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> index 17417220a083..ae90b718070a 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
> @@ -824,16 +824,16 @@ int main(int argc, char **argv)
> while ((opt = getopt(argc, argv, "hn:e:l:")) != -1) {
> switch (opt) {
> case 'n':
> - nr_irqs = atoi(optarg);
> + nr_irqs = atoi_paranoid(optarg);
> if (nr_irqs > 1024 || nr_irqs % 32)
> help(argv[0]);
> break;
> case 'e':
> - eoi_split = (bool)atoi(optarg);
> + eoi_split = (bool)atoi_paranoid(optarg);
> default_args = false;
> break;
> case 'l':
> - level_sensitive = (bool)atoi(optarg);
> + level_sensitive = (bool)atoi_paranoid(optarg);
> default_args = false;
> break;
> case 'h':
> diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c
> index 1c2749b1481a..99b16302d94d 100644
> --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c
> +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c
> @@ -361,7 +361,7 @@ int main(int argc, char *argv[])
> params.vcpu_memory_bytes = parse_size(optarg);
> break;
> case 'v':
> - params.nr_vcpus = atoi(optarg);
> + params.nr_vcpus = atoi_paranoid(optarg);
> break;
> case 'o':
> overlap_memory_access = true;
> diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> index 779ae54f89c4..82597fb04146 100644
> --- a/tools/testing/selftests/kvm/demand_paging_test.c
> +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> @@ -427,7 +427,7 @@ int main(int argc, char *argv[])
> p.src_type = parse_backing_src_type(optarg);
> break;
> case 'v':
> - nr_vcpus = atoi(optarg);
> + nr_vcpus = atoi_paranoid(optarg);
> TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
> "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
> break;
> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index acf8b80c91d1..1346f6b5a9bd 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> @@ -417,7 +417,7 @@ int main(int argc, char *argv[])
> dirty_log_manual_caps = 0;
> break;
> case 'f':
> - p.wr_fract = atoi(optarg);
> + p.wr_fract = atoi_paranoid(optarg);
> TEST_ASSERT(p.wr_fract >= 1,
> "Write fraction cannot be less than one");
> break;
> @@ -428,7 +428,7 @@ int main(int argc, char *argv[])
> help(argv[0]);
> break;
> case 'i':
> - p.iterations = atoi(optarg);
> + p.iterations = atoi_paranoid(optarg);
> break;
> case 'm':
> guest_modes_cmdline(optarg);
> @@ -446,12 +446,12 @@ int main(int argc, char *argv[])
> p.backing_src = parse_backing_src_type(optarg);
> break;
> case 'v':
> - nr_vcpus = atoi(optarg);
> + nr_vcpus = atoi_paranoid(optarg);
> TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
> "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
> break;
> case 'x':
> - p.slots = atoi(optarg);
> + p.slots = atoi_paranoid(optarg);
> break;
> default:
> help(argv[0]);
> diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
> index 5c5a88180b6c..56776f431733 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -150,4 +150,6 @@ static inline void *align_ptr_up(void *x, size_t size)
> return (void *)align_up((unsigned long)x, size);
> }
>
> +int atoi_paranoid(const char *num_str);
> +
> #endif /* SELFTEST_KVM_TEST_UTIL_H */
> diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c
> index f42c6ac6d71d..ea7feb69bb88 100644
> --- a/tools/testing/selftests/kvm/kvm_page_table_test.c
> +++ b/tools/testing/selftests/kvm/kvm_page_table_test.c
> @@ -461,7 +461,7 @@ int main(int argc, char *argv[])
> p.test_mem_size = parse_size(optarg);
> break;
> case 'v':
> - nr_vcpus = atoi(optarg);
> + nr_vcpus = atoi_paranoid(optarg);
> TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
> "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
> break;
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index 6d23878bbfe1..1e560c30a696 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -334,3 +334,17 @@ long get_run_delay(void)
>
> return val[1];
> }
> +
> +int atoi_paranoid(const char *num_str)
> +{
> + int num;
> + char *end_ptr;
> +
> + errno = 0;
> + num = (int)strtol(num_str, &end_ptr, 10);
> + TEST_ASSERT(errno == 0, "Conversion error: %d\n", errno);
"Conversion error: " is a bit vague. Also, TEST_ASSERT() already logs
errno and strerror(errno). It would probably be more useful here to log
the input string that caused the conversion error.
How about this?
TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str);
> + TEST_ASSERT(num_str != end_ptr && *end_ptr == '\0',
> + "Invalid number string.\n");
"Invalid number string." is also a bit vague. How about this?
TEST_ASSERT(num_str != end_ptr && *end_ptr == '\0',
"strtol(\"%s\") failed to parse trailing characters \"%s\"",
num_str, end_ptr);
> +
> + return num;
> +}
> diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
> index 9a6e4f3ad6b5..1595b73dc09a 100644
> --- a/tools/testing/selftests/kvm/max_guest_memory_test.c
> +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
> @@ -193,15 +193,15 @@ int main(int argc, char *argv[])
> while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) {
> switch (opt) {
> case 'c':
> - nr_vcpus = atoi(optarg);
> + nr_vcpus = atoi_paranoid(optarg);
> TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0");
> break;
> case 'm':
> - max_mem = atoi(optarg) * size_1gb;
> + max_mem = atoi_paranoid(optarg) * size_1gb;
> TEST_ASSERT(max_mem > 0, "memory size must be >0");
> break;
> case 's':
> - slot_size = atoi(optarg) * size_1gb;
> + slot_size = atoi_paranoid(optarg) * size_1gb;
> TEST_ASSERT(slot_size > 0, "slot size must be >0");
> break;
> case 'H':
> diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> index 6ee7e1dde404..865276993ffb 100644
> --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> @@ -166,7 +166,7 @@ int main(int argc, char *argv[])
> guest_percpu_mem_size = parse_size(optarg);
> break;
> case 'v':
> - nr_vcpus = atoi(optarg);
> + nr_vcpus = atoi_paranoid(optarg);
> TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
> "Invalid number of vcpus, must be between 1 and %d",
> max_vcpus);
> @@ -175,7 +175,7 @@ int main(int argc, char *argv[])
> p.partition_vcpu_memory_access = false;
> break;
> case 'i':
> - p.nr_memslot_modifications = atoi(optarg);
> + p.nr_memslot_modifications = atoi_paranoid(optarg);
> break;
> case 'h':
> default:
> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
> index 44995446d942..4bae9e3f5ca1 100644
> --- a/tools/testing/selftests/kvm/memslot_perf_test.c
> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c
> @@ -885,21 +885,21 @@ static bool parse_args(int argc, char *argv[],
> map_unmap_verify = true;
> break;
> case 's':
> - targs->nslots = atoi(optarg);
> + targs->nslots = atoi_paranoid(optarg);
> if (targs->nslots <= 0 && targs->nslots != -1) {
> pr_info("Slot count cap has to be positive or -1 for no cap\n");
> return false;
> }
> break;
> case 'f':
> - targs->tfirst = atoi(optarg);
> + targs->tfirst = atoi_paranoid(optarg);
> if (targs->tfirst < 0) {
> pr_info("First test to run has to be non-negative\n");
> return false;
> }
> break;
> case 'e':
> - targs->tlast = atoi(optarg);
> + targs->tlast = atoi_paranoid(optarg);
> if (targs->tlast < 0 || targs->tlast >= NTESTS) {
> pr_info("Last test to run has to be non-negative and less than %zu\n",
> NTESTS);
> @@ -907,14 +907,14 @@ static bool parse_args(int argc, char *argv[],
> }
> break;
> case 'l':
> - targs->seconds = atoi(optarg);
> + targs->seconds = atoi_paranoid(optarg);
> if (targs->seconds < 0) {
> pr_info("Test length in seconds has to be non-negative\n");
> return false;
> }
> break;
> case 'r':
> - targs->runs = atoi(optarg);
> + targs->runs = atoi_paranoid(optarg);
> if (targs->runs <= 0) {
> pr_info("Runs per test has to be positive\n");
> return false;
> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index 0d55f508d595..c366949c8362 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -407,7 +407,7 @@ int main(int argc, char *argv[])
>
> #ifdef __x86_64__
> if (argc > 1)
> - loops = atoi(argv[1]);
> + loops = atoi_paranoid(argv[1]);
> else
> loops = 10;
>
> diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> index cc6421716400..5e18d716782b 100644
> --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> @@ -233,10 +233,10 @@ int main(int argc, char **argv)
> while ((opt = getopt(argc, argv, "hp:t:r")) != -1) {
> switch (opt) {
> case 'p':
> - reclaim_period_ms = atoi(optarg);
> + reclaim_period_ms = atoi_paranoid(optarg);
> break;
> case 't':
> - token = atoi(optarg);
> + token = atoi_paranoid(optarg);
> break;
> case 'r':
> reboot_permissions = true;
> --
> 2.37.2.672.g94769d06f0-goog
>
next prev parent reply other threads:[~2022-09-21 18:14 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-26 18:44 [PATCH v3 0/4] dirty_log_perf_test cpu pinning and some goodies Vipin Sharma
2022-08-26 18:44 ` [PATCH v3 1/4] KVM: selftests: Explicitly set variables based on options in dirty_log_perf_test Vipin Sharma
2022-08-26 21:12 ` Vipin Sharma
2022-09-21 17:55 ` David Matlack
2022-08-26 18:44 ` [PATCH v3 2/4] KVM: selftests: Put command line options in alphabetical order " Vipin Sharma
2022-08-29 16:06 ` Andrew Jones
2022-08-26 18:44 ` [PATCH v3 3/4] KVM: selftests: Add atoi_paranoid to catch errors missed by atoi Vipin Sharma
2022-09-21 18:14 ` David Matlack [this message]
2022-08-26 18:45 ` [PATCH v3 4/4] KVM: selftests: Run dirty_log_perf_test on specific cpus Vipin Sharma
2022-09-21 19:02 ` David Matlack
2022-09-20 16:31 ` [PATCH v3 0/4] dirty_log_perf_test cpu pinning and some goodies Vipin Sharma
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YytUeOhWEBJF6MMz@google.com \
--to=dmatlack@google.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=vipinsh@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.