Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Tim Wawrzynczak, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 16: Code-Review+1
(2 comments)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/603f9994_2a14a542 PS13, Line 666: if (!dump_coremsrs) {
Do you agree with this? Resolved?
Sometimes, the best way to solve a problem is to remove what causes the problem to exist. 😄
(Yes, I agree)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/93cd719b_7d22a525 PS16, Line 671: if (sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end) == 1) { : /* the end of the range is not specified - only for one core */ : cores_range_end = cores_range_start; : } else if (cores_range_end < cores_range_start) { : printf("Error: invalid cores range <%u-%u>!\n", : cores_range_start, cores_range_end); : exit(1); : } I was thinking of something like this:
const int sscanf_ret = sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end); if (sscanf_ret == 1) { /* the end of the range is not specified - only for one core */ cores_range_end = cores_range_start; } else if (sscanf_ret != 2) { print_usage(argv[0]); exit(1); } else if (cores_range_end < cores_range_start) { printf("Error: invalid cores range <%u-%u>!\n", cores_range_start, cores_range_end); exit(1); }
The main difference is the addition of the second check, `sscanf_ret != 2`. If true, it means the specified argument format isn't correct, and we shouldn't rely on the numeric values. I'm not sure when a negative number would be returned (if possible), but the return value can very well be zero: when the argument doesn't match the expected format and no numbers were scanned.