Bug 1835452
| Summary: | Review Request: mlxbf-bootctl - Bootloader control for Mellanox BlueField | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Spencer Lingard <spencer> |
| Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
| Status: | CLOSED NOTABUG | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | rawhide | CC: | ahleihel, honli, jfeeney, mschmidt, package-review, pbunyan |
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2022-06-21 00:45:16 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 201449, 1656147, 1853081 | ||
|
Description
Spencer Lingard
2020-05-13 20:00:22 UTC
New version for review. Spec URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01441117-mlxbf-bootctl/mlxbf-bootctl.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01441117-mlxbf-bootctl/mlxbf-bootctl-1.1-4.fc33.src.rpm Scratch Koji URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=45583728 1 Name: mlxbf-bootctl
2 Version: 1.1
3 %{!?_release: %define _release 4}
4 Release: %{_release}%{?dist}
Please delete line 3, and replace "%{_release}" with 4 for line 4.
5 Summary: Mellanox BlueField boot partition management utility
6
7 License: BSD
8 Url: https://github.com/Mellanox/mlxbf-bootctl
9 Source: mlxbf-bootctl-1.1.tar.gz
10
11 ExclusiveArch: aarch64
Need a comment for "ExclusiveArch", see https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
12
13 BuildRequires: binutils
14 BuildRequires: gcc
line 13 should be deleted, as gcc requires binutils.
$ rpm -qR gcc | grep binutils
binutils >= 2.31
15
16 %description
17 Access to all the boot partition management is via a program shipped
18 with the BlueField software called "mlxbf-bootctl".
I have no idea what is main function or feature of this package after read this
'%description' section. Please improve it.
19
20 %prep
21 %setup -q -n mlxbf-bootctl-1.1
"%setup -q" should be enough, in case
1) top directory name was in format "%{name}-%{version}/"
2) tarball name was in format "%{name}-%{version}.XXX"
22
23 %build
24 %make_build
25
26 %install
27 %make_install
28 %{__install} -d %{buildroot}%{_mandir}/man8
29 %{__install} -m 0644 mlxbf-bootctl.8 %{buildroot}%{_mandir}/man8
30
31 %files
32 %defattr(-, root, root)
line 32 is unnecessary, please remove it.
33 /sbin/*
should install programs in %{_sbindir}, and use %{_sbindir}/XXX, XXX is the program name.
34 %{_mandir}/man8/mlxbf-bootctl.8.gz
35
36 %license LICENSE
37 %doc mlxbf-bootctl.txt
38
39 %changelog
40 * Wed Jun 10 2020 Spencer Lingard <spencer> 1.1-4
41 (none)
42
43 * Tue May 12 2020 Spencer Lingard <spencer> 1.1-3
44 (none)
line 41 and 44 are unnecessary, should be deleted.
Task URL: https://cov01.lab.eng.brq.redhat.com/covscanhub/task/175416/ Comment: None All defects CHECKED_RETURN 1 CLANG_WARNING 1 CPPCHECK_WARNING 1 ========================================================================== mlxbf-bootctl-1.1-4.fc31 List of Defects Error: CPPCHECK_WARNING (CWE-664): [#def1] mlxbf-bootctl-1.1/mlxbf-bootctl.c:60: error[va_end_missing]: va_list 'ap' was opened but not closed by va_end(). # 58| putc('\n', stderr); # 59| exit(1); # 60|-> } # 61| # 62| #ifndef OUTPUT_ONLY Error: CLANG_WARNING: [#def2] mlxbf-bootctl-1.1/mlxbf-bootctl.c:545:5: warning: Null pointer passed to 1st parameter expecting 'nonnull' # memset(buf + seg_size, 0, pad_size); # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:10: note: Assuming the condition is true # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:3: note: Loop condition is true. Entering loop body # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:647:5: note: Control jumps to 'case 98:' at line 663 # switch (opt) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:665:7: note: Execution continues on line 644 # break; # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:10: note: Assuming the condition is false # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:3: note: Loop condition is false. Execution continues on line 694 # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:694:7: note: Assuming 'bootstream' is non-null # if (!bootstream && !swap && watchdog_swap == NULL && !watchdog_disable) # ^~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:694:19: note: Left side of '&&' is false # if (!bootstream && !swap && watchdog_swap == NULL && !watchdog_disable) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:700:7: note: 'bootstream' is non-null # if (bootstream) # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:700:3: note: Taking true branch # if (bootstream) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:702:9: note: 'input_file' is null # if (input_file) # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:702:5: note: Taking false branch # if (input_file) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:707:14: note: 'output_file' is null # else if (output_file) # ^~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:707:10: note: Taking false branch # else if (output_file) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:718:11: note: Assuming 'boot_part_size' is >= field 'st_size' # if (st.st_size > boot_part_size) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:718:7: note: Taking false branch # if (st.st_size > boot_part_size) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:725:11: note: Assuming the condition is false # if (asprintf(&bootfile, "%sboot%d", mmc_path, boot_part ^ which_boot) <= 0) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:725:7: note: Taking false branch # if (asprintf(&bootfile, "%sboot%d", mmc_path, boot_part ^ which_boot) <= 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:728:7: note: Calling 'write_bootstream' # write_bootstream(bootstream, bootfile, O_SYNC); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:475:3: note: Taking true branch # if (strncmp(bootfile, "/dev/", 5) == 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:477:9: note: Assuming the condition is false # if (asprintf(&sysname, "/sys/block/%s/force_ro", &bootfile[5]) <= 0) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:477:5: note: Taking false branch # if (asprintf(&sysname, "/sys/block/%s/force_ro", &bootfile[5]) <= 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:481:9: note: Assuming 'sysfd' is < 0 # if (sysfd >= 0) # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:481:5: note: Taking false branch # if (sysfd >= 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:503:11: note: Assuming the condition is false # if (errno != ENOENT) # ^~~~~~~~~~~~~~~ /usr/include/errno.h:38:16: note: expanded from macro 'errno' ## define errno (*__errno_location ()) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:503:7: note: Taking false branch # if (errno != ENOENT) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:513:7: note: Assuming 'ifd' is < 0 # if (ifd < 0) # ^~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:513:3: note: Taking true branch # if (ifd < 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:516:7: note: Assuming 'ofd' is >= 0 # if (ofd < 0) # ^~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:516:3: note: Taking false branch # if (ofd < 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:519:7: note: Assuming the condition is false # if (fstat(ifd, &st) < 0) # ^~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:519:3: note: Taking false branch # if (fstat(ifd, &st) < 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:523:3: note: 'buf' initialized here # char *buf = malloc(MAX_SEG_LEN); # ^~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:524:7: note: Assuming 'buf' is equal to NULL # if (buf == NULL) # ^~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:524:3: note: Taking true branch # if (buf == NULL) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:532:10: note: Assuming 'bytes_left' is > 0 # while (bytes_left > 0) # ^~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:532:3: note: Loop condition is true. Entering loop body # while (bytes_left > 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:534:24: note: Assuming the condition is false # size_t seg_size = (bytes_left <= MAX_SEG_LEN) ? bytes_left : MAX_SEG_LEN; # ^~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:534:23: note: '?' condition is false # size_t seg_size = (bytes_left <= MAX_SEG_LEN) ? bytes_left : MAX_SEG_LEN; # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:538:23: note: '?' condition is false # size_t pad_size = seg_size % 8 ? (8 - seg_size % 8) : 0; # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:539:41: note: 'bytes_left' is not equal to 0 # uint64_t segheader = gen_seg_header(bytes_left == 0, 1, BOOT_FIFO_ADDR, # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:545:5: note: Null pointer passed to 1st parameter expecting 'nonnull' # memset(buf + seg_size, 0, pad_size); # ^ ~~~~~~~~~~~~~~ # 543| // Copy the segment plus any padding. # 544| read_or_die(bootstream, ifd, buf, seg_size); # 545|-> memset(buf + seg_size, 0, pad_size); # 546| write_or_die(bootfile, ofd, buf, seg_size + pad_size); # 547| } Error: CHECKED_RETURN (CWE-252): [#def3] mlxbf-bootctl-1.1/mlxbf-bootctl.c:717: check_return: Calling "stat(bootstream, &st)" without checking return value. This library function may fail and return an error code. [Note: The source code implementation of the function has been overridden by a builtin model.] # 715| uint64_t boot_part_size = get_boot_partition_size(); # 716| struct stat st; # 717|-> stat(bootstream, &st); # 718| if (st.st_size > boot_part_size) # 719| die("Size of bootstream exceeds boot partition size"); Hi, Alaa Do we have hardware to test this package? Thanks (In reply to Honggang LI from comment #4) > Hi, Alaa > Do we have hardware to test this package? Thanks Hi, Honggang. Yes, we can use the existing BlueField card in the lab; the same one we used for reviewing the rshim package. Regards, Alaa Spec URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01476093-mlxbf-bootctl/mlxbf-bootctl.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01476093-mlxbf-bootctl/mlxbf-bootctl-1.1-5.fc33.src.rpm Scratch Koji URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=45835273 New version for review. (In reply to Honggang LI from comment #2) > 1 Name: mlxbf-bootctl > 2 Version: 1.1 > 3 %{!?_release: %define _release 4} > 4 Release: %{_release}%{?dist} > Please delete line 3, and replace "%{_release}" with 4 for line 4. Done. > > 5 Summary: Mellanox BlueField boot partition management utility > 6 > 7 License: BSD > 8 Url: https://github.com/Mellanox/mlxbf-bootctl > 9 Source: mlxbf-bootctl-1.1.tar.gz > 10 > 11 ExclusiveArch: aarch64 > > Need a comment for "ExclusiveArch", see > https://fedoraproject.org/wiki/Packaging:ReviewGuidelines Done. > > 12 > 13 BuildRequires: binutils > 14 BuildRequires: gcc > > line 13 should be deleted, as gcc requires binutils. > > $ rpm -qR gcc | grep binutils > binutils >= 2.31 Done. > > 15 > 16 %description > 17 Access to all the boot partition management is via a program shipped > 18 with the BlueField software called "mlxbf-bootctl". > > I have no idea what is main function or feature of this package after read > this > '%description' section. Please improve it. Done. > > 19 > 20 %prep > 21 %setup -q -n mlxbf-bootctl-1.1 > > "%setup -q" should be enough, in case > 1) top directory name was in format "%{name}-%{version}/" > 2) tarball name was in format "%{name}-%{version}.XXX" This spec file is generated by rpkg (https://github.com/Mellanox/mlxbf-bootctl/blob/master/mlxbf-bootctl.spec.rpkg). As far as I can tell, I can't change the way rpkg generates the %setup macro. > > 22 > 23 %build > 24 %make_build > 25 > 26 %install > 27 %make_install > 28 %{__install} -d %{buildroot}%{_mandir}/man8 > 29 %{__install} -m 0644 mlxbf-bootctl.8 %{buildroot}%{_mandir}/man8 > 30 > 31 %files > 32 %defattr(-, root, root) > line 32 is unnecessary, please remove it. Done. > > 33 /sbin/* > should install programs in %{_sbindir}, and use %{_sbindir}/XXX, XXX is the > program name. Done. > > 34 %{_mandir}/man8/mlxbf-bootctl.8.gz > 35 > 36 %license LICENSE > 37 %doc mlxbf-bootctl.txt > 38 > 39 %changelog > 40 * Wed Jun 10 2020 Spencer Lingard <spencer> 1.1-4 > 41 (none) > 42 > 43 * Tue May 12 2020 Spencer Lingard <spencer> 1.1-3 > 44 (none) > > line 41 and 44 are unnecessary, should be deleted. Changed them to be more detailed. (In reply to Honggang LI from comment #3) > Task URL: https://cov01.lab.eng.brq.redhat.com/covscanhub/task/175416/ > Comment: None > > > All defects > > CHECKED_RETURN 1 > CLANG_WARNING 1 > CPPCHECK_WARNING 1 > > ========================================================================== > mlxbf-bootctl-1.1-4.fc31 > List of Defects > > Error: CPPCHECK_WARNING (CWE-664): [#def1] > mlxbf-bootctl-1.1/mlxbf-bootctl.c:60: error[va_end_missing]: va_list 'ap' > was opened but not closed by va_end(). > # 58| putc('\n', stderr); > # 59| exit(1); > # 60|-> } > # 61| > # 62| #ifndef OUTPUT_ONLY Fixed. > > Error: CLANG_WARNING: [#def2] > mlxbf-bootctl-1.1/mlxbf-bootctl.c:545:5: warning: Null pointer passed to 1st > parameter expecting 'nonnull' > # memset(buf + seg_size, 0, pad_size); > # ^ This is a false positive. The tool assumes that (buf == NULL) at line 524, but if buf == NULL, the program will die with an error message rather than continuing. See the note below. > mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:10: note: Assuming the condition is > true > # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) > # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:3: note: Loop condition is true. > Entering loop body > # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:647:5: note: Control jumps to 'case 98:' > at line 663 > # switch (opt) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:665:7: note: Execution continues on line > 644 > # break; > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:10: note: Assuming the condition is > false > # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) > # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:3: note: Loop condition is false. > Execution continues on line 694 > # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:694:7: note: Assuming 'bootstream' is > non-null > # if (!bootstream && !swap && watchdog_swap == NULL && !watchdog_disable) > # ^~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:694:19: note: Left side of '&&' is false > # if (!bootstream && !swap && watchdog_swap == NULL && !watchdog_disable) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:700:7: note: 'bootstream' is non-null > # if (bootstream) > # ^~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:700:3: note: Taking true branch > # if (bootstream) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:702:9: note: 'input_file' is null > # if (input_file) > # ^~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:702:5: note: Taking false branch > # if (input_file) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:707:14: note: 'output_file' is null > # else if (output_file) > # ^~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:707:10: note: Taking false branch > # else if (output_file) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:718:11: note: Assuming 'boot_part_size' is > >= field 'st_size' > # if (st.st_size > boot_part_size) > # ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:718:7: note: Taking false branch > # if (st.st_size > boot_part_size) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:725:11: note: Assuming the condition is > false > # if (asprintf(&bootfile, "%sboot%d", mmc_path, boot_part ^ which_boot) > <= 0) > # > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:725:7: note: Taking false branch > # if (asprintf(&bootfile, "%sboot%d", mmc_path, boot_part ^ which_boot) > <= 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:728:7: note: Calling 'write_bootstream' > # write_bootstream(bootstream, bootfile, O_SYNC); > # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:475:3: note: Taking true branch > # if (strncmp(bootfile, "/dev/", 5) == 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:477:9: note: Assuming the condition is > false > # if (asprintf(&sysname, "/sys/block/%s/force_ro", &bootfile[5]) <= 0) > # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:477:5: note: Taking false branch > # if (asprintf(&sysname, "/sys/block/%s/force_ro", &bootfile[5]) <= 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:481:9: note: Assuming 'sysfd' is < 0 > # if (sysfd >= 0) > # ^~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:481:5: note: Taking false branch > # if (sysfd >= 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:503:11: note: Assuming the condition is > false > # if (errno != ENOENT) > # ^~~~~~~~~~~~~~~ > /usr/include/errno.h:38:16: note: expanded from macro 'errno' > ## define errno (*__errno_location ()) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:503:7: note: Taking false branch > # if (errno != ENOENT) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:513:7: note: Assuming 'ifd' is < 0 > # if (ifd < 0) > # ^~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:513:3: note: Taking true branch > # if (ifd < 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:516:7: note: Assuming 'ofd' is >= 0 > # if (ofd < 0) > # ^~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:516:3: note: Taking false branch > # if (ofd < 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:519:7: note: Assuming the condition is > false > # if (fstat(ifd, &st) < 0) > # ^~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:519:3: note: Taking false branch > # if (fstat(ifd, &st) < 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:523:3: note: 'buf' initialized here > # char *buf = malloc(MAX_SEG_LEN); > # ^~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:524:7: note: Assuming 'buf' is equal to > NULL > # if (buf == NULL) > # ^~~~~~~~~~~ Note: if buf == NULL here, the true branch will stop the program, and a NULL pointer will not be passed to memset. > mlxbf-bootctl-1.1/mlxbf-bootctl.c:524:3: note: Taking true branch > # if (buf == NULL) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:532:10: note: Assuming 'bytes_left' is > 0 > # while (bytes_left > 0) > # ^~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:532:3: note: Loop condition is true. > Entering loop body > # while (bytes_left > 0) > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:534:24: note: Assuming the condition is > false > # size_t seg_size = (bytes_left <= MAX_SEG_LEN) ? bytes_left : > MAX_SEG_LEN; > # ^~~~~~~~~~~~~~~~~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:534:23: note: '?' condition is false > # size_t seg_size = (bytes_left <= MAX_SEG_LEN) ? bytes_left : > MAX_SEG_LEN; > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:538:23: note: '?' condition is false > # size_t pad_size = seg_size % 8 ? (8 - seg_size % 8) : 0; > # ^ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:539:41: note: 'bytes_left' is not equal to > 0 > # uint64_t segheader = gen_seg_header(bytes_left == 0, 1, BOOT_FIFO_ADDR, > # ^~~~~~~~~~ > mlxbf-bootctl-1.1/mlxbf-bootctl.c:545:5: note: Null pointer passed to 1st > parameter expecting 'nonnull' > # memset(buf + seg_size, 0, pad_size); > # ^ ~~~~~~~~~~~~~~ > # 543| // Copy the segment plus any padding. > # 544| read_or_die(bootstream, ifd, buf, seg_size); > # 545|-> memset(buf + seg_size, 0, pad_size); > # 546| write_or_die(bootfile, ofd, buf, seg_size + pad_size); > # 547| } > > Error: CHECKED_RETURN (CWE-252): [#def3] > mlxbf-bootctl-1.1/mlxbf-bootctl.c:717: check_return: Calling > "stat(bootstream, &st)" without checking return value. This library function > may fail and return an error code. [Note: The source code implementation of > the function has been overridden by a builtin model.] > # 715| uint64_t boot_part_size = get_boot_partition_size(); > # 716| struct stat st; > # 717|-> stat(bootstream, &st); > # 718| if (st.st_size > boot_part_size) > # 719| die("Size of bootstream exceeds boot partition size"); Fixed. Confirmed all issues found by Coverity had been fixed in https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01476093-mlxbf-bootctl/mlxbf-bootctl-1.1-5.fc33.src.rpm . Thanks (In reply to Spencer Lingard from comment #6) > Spec URL: > https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/ > fedora-rawhide-aarch64/01476093-mlxbf-bootctl/mlxbf-bootctl.spec 27 %build 28 %make_build Need to setup the CFLAGS. Please insert %set_build_flags between line 27 and 28. see https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/master/f/buildflags.md https://kojipkgs.fedoraproject.org//work/tasks/5292/45835292/build.log + cd mlxbf-bootctl-1.1 + /usr/bin/make -O -j5 V=1 VERBOSE=1 cc -O2 -g -std=gnu99 -Werror -Wall -Wshadow -Wuninitialized -Wstrict-overflow -Wundef -Wold-style-definition -Wwrite-strings -MD -MP -c -o mlxbf-bootctl.o mlxbf-bootctl.c cc -Wl,--fatal-warnings -o mlxbf-bootctl mlxbf-bootctl.o @Michal Could you please have a look? I'm almost OK with the spec file. I had checked 'fedora-review' result for this request. It looks almost good. If you are OK with the spec file. I will set the 'fedora-review+' flag, and post the fedora-review result.txt. Thanks I ran some tests, it seems OK:
[root@dhcp19-243-132 ~]# rpm -qf `which mlxbf-bootctl`
mlxbf-bootctl-1.1-5.el8.aarch64
[root@dhcp19-243-132 ~]#
[root@dhcp19-243-132 ~]# mlxbf-bootctl --help
syntax: mlxbf-bootctl [--help|-h] [--swap|-s] [--device|-d MMCFILE]
[--output|-o OUTPUT] [--read|-r INPUT]
[--bootstream|-b BFBFILE] [--overwrite-current]
[--watchdog-swap interval | --nowatchdog-swap]
[root@dhcp19-243-132 ~]# modprobe -rv mlxbf_bootctl
rmmod mlxbf_bootctl
[root@dhcp19-243-132 ~]# mlxbf-bootctl
primary: /dev/mmcblk0boot0
backup: /dev/mmcblk0boot1
boot-bus-width: x8
reset to x1 after reboot: FALSE
post_reset_wdog: No such file or directory
[root@dhcp19-243-132 ~]# echo $?
1
[root@dhcp19-243-132 ~]# modprobe -v mlxbf_bootctl
insmod /lib/modules/4.18.0-215.el8.aarch64/kernel/drivers/platform/mellanox/mlxbf-bootctl.ko.xz
[root@dhcp19-243-132 ~]# mlxbf-bootctl
primary: /dev/mmcblk0boot0
backup: /dev/mmcblk0boot1
boot-bus-width: x8
reset to x1 after reboot: FALSE
watchdog-swap: disabled
lifecycle state: GA Non-Secured
secure boot key free slots: 4
[root@dhcp19-243-132 ~]# echo $?
0
[root@dhcp19-243-132 ~]#
Reading and writing boot partitions:
[root@dhcp19-243-132 ~]# mlxbf-bootctl -r /dev/mmcblk0boot0 -b boot0.bfb
Copy bootstream from /dev/mmcblk0boot0 to boot0.bfb
[root@dhcp19-243-132 ~]# ls -lh boot0.bfb
-rw-r--r--. 1 root root 2.1M Jun 18 10:49 boot0.bfb
[root@dhcp19-243-132 ~]#
[root@dhcp19-243-132 ~]# mlxbf-bootctl -r /dev/mmcblk0boot1 -b boot1.bfb
Copy bootstream from /dev/mmcblk0boot1 to boot1.bfb
[root@dhcp19-243-132 ~]# ls -lh boot1.bfb
-rw-r--r--. 1 root root 1.6M Jun 18 10:59 boot1.bfb
[root@dhcp19-243-132 ~]#
[root@dhcp19-243-132 ~]# mlxbf-bootctl --bootstream boot0.bfb --swap --watchdog-swap 60
[root@dhcp19-243-132 ~]# reboot
enable disable watchdog-swap:
[root@dhcp19-243-132 ~]# mlxbf-bootctl
primary: /dev/mmcblk0boot1
backup: /dev/mmcblk0boot0
boot-bus-width: x8
reset to x1 after reboot: FALSE
watchdog-swap: disabled
lifecycle state: GA Non-Secured
secure boot key free slots: 4
[root@dhcp19-243-132 ~]# mlxbf-bootctl --watchdog-swap 60
[root@dhcp19-243-132 ~]# mlxbf-bootctl
primary: /dev/mmcblk0boot1
backup: /dev/mmcblk0boot0
boot-bus-width: x8
reset to x1 after reboot: FALSE
watchdog-swap: 60
lifecycle state: GA Non-Secured
secure boot key free slots: 4
[root@dhcp19-243-132 ~]# mlxbf-bootctl --nowatchdog-swap
[root@dhcp19-243-132 ~]# mlxbf-bootctl
primary: /dev/mmcblk0boot1
backup: /dev/mmcblk0boot0
boot-bus-width: x8
reset to x1 after reboot: FALSE
watchdog-swap: disabled
lifecycle state: GA Non-Secured
secure boot key free slots: 4
[root@dhcp19-243-132 ~]#
New version. Spec URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01496412-mlxbf-bootctl/mlxbf-bootctl.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01496412-mlxbf-bootctl/mlxbf-bootctl-1.1-6.fc33.src.rpm Scratch Koji URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=46001191 (In reply to Honggang LI from comment #8) > (In reply to Spencer Lingard from comment #6) > > Spec URL: > > https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/ > > fedora-rawhide-aarch64/01476093-mlxbf-bootctl/mlxbf-bootctl.spec > > 27 %build > 28 %make_build > > Need to setup the CFLAGS. Please insert %set_build_flags between line 27 and > 28. see > https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/master/f/ > buildflags.md > Ok, done. I see the message "annobin: mlxbf-bootctl.c: ICE: Should be 64-bit target" when I build with these flags. Is this expected? The binaries appear to run fine. (In reply to Alaa Hleihel (Mellanox) from comment #9) > I ran some tests, it seems OK: > > > [root@dhcp19-243-132 ~]# rpm -qf `which mlxbf-bootctl` > mlxbf-bootctl-1.1-5.el8.aarch64 > [root@dhcp19-243-132 ~]# > [root@dhcp19-243-132 ~]# mlxbf-bootctl --help > syntax: mlxbf-bootctl [--help|-h] [--swap|-s] [--device|-d MMCFILE] > [--output|-o OUTPUT] [--read|-r INPUT] > [--bootstream|-b BFBFILE] [--overwrite-current] > [--watchdog-swap interval | --nowatchdog-swap] > > > [root@dhcp19-243-132 ~]# modprobe -rv mlxbf_bootctl > rmmod mlxbf_bootctl > [root@dhcp19-243-132 ~]# mlxbf-bootctl > primary: /dev/mmcblk0boot0 > backup: /dev/mmcblk0boot1 > boot-bus-width: x8 > reset to x1 after reboot: FALSE > post_reset_wdog: No such file or directory > [root@dhcp19-243-132 ~]# echo $? > 1 > [root@dhcp19-243-132 ~]# modprobe -v mlxbf_bootctl > insmod > /lib/modules/4.18.0-215.el8.aarch64/kernel/drivers/platform/mellanox/mlxbf- > bootctl.ko.xz > [root@dhcp19-243-132 ~]# mlxbf-bootctl > primary: /dev/mmcblk0boot0 > backup: /dev/mmcblk0boot1 > boot-bus-width: x8 > reset to x1 after reboot: FALSE > watchdog-swap: disabled > lifecycle state: GA Non-Secured > secure boot key free slots: 4 > [root@dhcp19-243-132 ~]# echo $? > 0 > [root@dhcp19-243-132 ~]# For completeness I added a message that warns if you don't have the mlxbf_bootctl driver loaded, rather than printing "No such file or directory". 1 Package Review
2 ==============
3
4 Legend:
5 [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
6 [ ] = Manual review needed
7
8
9
10 ===== MUST items =====
11
12 C/C++:
13 [ ]: Package does not contain kernel modules.
PASS
14 [ ]: Package contains no static executables.
PASS
15 [x]: If your application is a C or C++ application you must list a
16 BuildRequires against gcc, gcc-c++ or clang.
17 [x]: Package does not contain any libtool archives (.la)
18 [x]: Rpath absent or only used for internal libs.
19
20 Generic:
21 [ ]: Package is licensed with an open-source compatible license and meets
22 other legal requirements as defined in the legal section of Packaging
23 Guidelines.
PASS
24 [ ]: License field in the package spec file matches the actual license.
25 Note: Checking patched sources after %prep for licenses. Licenses
26 found: "BSD 2-clause "Simplified" License", "Unknown or generated",
27 "GNU General Public License, Version 2". 11 files have unknown
28 license. Detailed output of licensecheck in /home/test/1835452-mlxbf-
29 bootctl/licensecheck.txt
PASS
30 [ ]: License file installed when any subpackage combination is installed.
PASS
31 [ ]: %build honors applicable compiler flags or justifies otherwise.
PASS
32 [ ]: Package contains no bundled libraries without FPC exception.
PASS
33 [ ]: Changelog in prescribed format.
PASS
34 [ ]: Sources contain only permissible code or content.
PASS
35 [ ]: Package contains desktop file if it is a GUI application.
PASS
36 [ ]: Development files must be in a -devel package
PASS
37 [ ]: Package uses nothing in %doc for runtime.
PASS
38 [ ]: Package consistently uses macros (instead of hard-coded directory
39 names).
PASS
40 [ ]: Package is named according to the Package Naming Guidelines.
PASS
41 [ ]: Package does not generate any conflict.
PASS
42 [ ]: Package obeys FHS, except libexecdir and /usr/target.
PASS
43 [ ]: If the package is a rename of another package, proper Obsoletes and
44 Provides are present.
PASS
45 [ ]: Requires correct, justified where necessary.
PASS
46 [ ]: Spec file is legible and written in American English.
PASS
47 [ ]: Package contains systemd file(s) if in need.
PASS
48 [ ]: Useful -debuginfo package or justification otherwise.
PASS
49 [ ]: Package is not known to require an ExcludeArch tag.
PASS. It is aarch64 specific package.
50 [ ]: Large documentation must go in a -doc subpackage. Large could be size
51 (~1MB) or number of files.
52 Note: Documentation size is 20480 bytes in 1 files.
PASS
53 [ ]: Package complies to the Packaging Guidelines
PASS
54 [x]: Package successfully compiles and builds into binary rpms on at least
55 one supported primary architecture.
56 [x]: Package installs properly.
57 [x]: Rpmlint is run on all rpms the build produces.
58 Note: There are rpmlint messages (see attachment).
59 [x]: If (and only if) the source package includes the text of the
60 license(s) in its own file, then that file, containing the text of the
61 license(s) for the package is included in %license.
62 [x]: Package requires other packages for directories it uses.
63 [x]: Package must own all directories that it creates.
64 [x]: Package does not own files or directories owned by other packages.
65 [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
66 [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
67 beginning of %install.
68 [x]: Macros in Summary, %description expandable at SRPM build time.
69 [x]: Dist tag is present.
70 [x]: Package does not contain duplicates in %files.
71 [x]: Permissions on files are set properly.
72 [x]: Package must not depend on deprecated() packages.
73 [x]: Package use %makeinstall only when make install DESTDIR=... doesn't
74 work.
75 [x]: Package is named using only allowed ASCII characters.
76 [x]: Package does not use a name that already exists.
77 [x]: Package is not relocatable.
78 [x]: Sources used to build the package match the upstream source, as
79 provided in the spec URL.
80 [x]: Spec file name must match the spec package %{name}, in the format
81 %{name}.spec.
82 [x]: File names are valid UTF-8.
83 [x]: Packages must not store files under /srv, /opt or /usr/local
84
85 ===== SHOULD items =====
86
87 Generic:
88 [ ]: If the source package does not include license text(s) as a separate
89 file from upstream, the packager SHOULD query upstream to include it.
PASS
90 [ ]: Final provides and requires are sane (see attachments).
PASS
91 [ ]: Package functions as described.
PASS
92 [ ]: Latest version is packaged.
PASS
93 [ ]: Package does not include license text files separate from upstream.
PASS
94 [ ]: SourceX tarball generation or download is documented.
95 Note: Package contains tarball without URL, check comments
PASS
96 [ ]: Sources are verified with gpgverify first in %prep if upstream
97 publishes signatures.
98 Note: gpgverify is not used.
PASS
99 [ ]: Description and summary sections in the package spec file contains
100 translations for supported Non-English languages, if available.
PASS
101 [ ]: %check is present and all tests pass.
PASS
102 [ ]: Packages should try to preserve timestamps of original installed
103 files.
PASS
104 [x]: Reviewer should test that the package builds in mock.
105 [x]: Buildroot is not present
106 [x]: Package has no %clean section with rm -rf %{buildroot} (or
107 $RPM_BUILD_ROOT)
108 [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
109 [x]: Fully versioned dependency in subpackages if applicable.
110 [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
111 [x]: SourceX is a working URL.
112 [x]: Package should compile and build into binary rpms on all supported
113 architectures.
114 [x]: Spec use %global instead of %define unless justified.
115
116 ===== EXTRA items =====
117
118 Generic:
119 [x]: Rpmlint is run on debuginfo package(s).
120 Note: No rpmlint messages.
121 [x]: Rpmlint is run on all installed packages.
122 Note: There are rpmlint messages (see attachment).
123 [x]: Large data in /usr/share should live in a noarch subpackage if package
124 is arched.
125 [x]: Spec file according to URL is the same as in SRPM.
126
127
128 Rpmlint
129 -------
130 Checking: mlxbf-bootctl-1.1-6.fc33.aarch64.rpm
131 mlxbf-bootctl-debuginfo-1.1-6.fc33.aarch64.rpm
132 mlxbf-bootctl-debugsource-1.1-6.fc33.aarch64.rpm
133 mlxbf-bootctl-1.1-6.fc33.src.rpm
134 mlxbf-bootctl.aarch64: W: spelling-error Summary(en_US) Mellanox -> Melanoma
135 mlxbf-bootctl.src: W: spelling-error Summary(en_US) Mellanox -> Melanoma
136 mlxbf-bootctl.src: W: invalid-url Source0: mlxbf-bootctl-1.1.tar.gz
137 4 packages and 0 specfiles checked; 0 errors, 3 warnings.
138
139
140
141
142 Rpmlint (debuginfo)
143 -------------------
144 Checking: mlxbf-bootctl-debuginfo-1.1-6.fc33.aarch64.rpm
145 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
146
147
148
149
150
151 Rpmlint (installed packages)
152 ----------------------------
153 mlxbf-bootctl-debuginfo.aarch64: W: invalid-url URL: https://github.com/Mellanox/mlxbf-bootctl <urlopen error [Errno -2] Name or service not known>
154 mlxbf-bootctl-debugsource.aarch64: W: invalid-url URL: https://github.com/Mellanox/mlxbf-bootctl <urlopen error [Errno -2] Name or service not known>
155 mlxbf-bootctl.aarch64: W: spelling-error Summary(en_US) Mellanox -> Melanoma
156 mlxbf-bootctl.aarch64: W: invalid-url URL: https://github.com/Mellanox/mlxbf-bootctl <urlopen error [Errno -2] Name or service not known>
157 3 packages and 0 specfiles checked; 0 errors, 4 warnings.
158
159
160
161 Requires
162 --------
163 mlxbf-bootctl (rpmlib, GLIBC filtered):
164 ld-linux-aarch64.so.1()(64bit)
165 libc.so.6()(64bit)
166 rtld(GNU_HASH)
167
168 mlxbf-bootctl-debuginfo (rpmlib, GLIBC filtered):
169
170 mlxbf-bootctl-debugsource (rpmlib, GLIBC filtered):
171
172
173
174 Provides
175 --------
176 mlxbf-bootctl:
177 mlxbf-bootctl
178 mlxbf-bootctl(aarch-64)
179
180 mlxbf-bootctl-debuginfo:
181 debuginfo(build-id)
182 mlxbf-bootctl-debuginfo
183 mlxbf-bootctl-debuginfo(aarch-64)
184
185 mlxbf-bootctl-debugsource:
186 mlxbf-bootctl-debugsource
187 mlxbf-bootctl-debugsource(aarch-64)
188
189
190
191 Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16
192 Command line :/usr/bin/fedora-review -b 1835452
193 Buildroot used: fedora-rawhide-aarch64
194 Active plugins: Generic, C/C++, Shell-api
195 Disabled plugins: Haskell, Perl, SugarActivity, Ruby, Python, Java, Ocaml, fonts, R, PHP
196 Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
(In reply to Spencer Lingard from comment #10) > Ok, done. I see the message > > "annobin: mlxbf-bootctl.c: ICE: Should be 64-bit target" > > when I build with these flags. Is this expected? That's OK. I did not see such message when I built it with mock fedora-rawhide configuration. Hi, What is the status of adding this package to Fedora? Thanks, Alaa (In reply to Alaa Hleihel (NVIDIA Mellanox) from comment #13) > Hi, > > What is the status of adding this package to Fedora? https://bugzilla.redhat.com/show_bug.cgi?id=1846139#c3 It seems the firmware will be rejected. Is it still necessary to package up this tool for Fedora? (In reply to Honggang LI from comment #14) > (In reply to Alaa Hleihel (NVIDIA Mellanox) from comment #13) > > Hi, > > > > What is the status of adding this package to Fedora? > > https://bugzilla.redhat.com/show_bug.cgi?id=1846139#c3 > > It seems the firmware will be rejected. Is it still necessary to package up > this tool for Fedora? Hi, Honggang. It's not clear yet how the FW images in BZ #1846139 will be shipped. But regardless, I think that this package should still be added to Fedora, it provide other functionalities for managing the boot partitions and features. Got it. As the SPEC and SRPM URLs are no longer valid, please update the URLs. (In reply to Honggang LI from comment #16) > Got it. As the SPEC and SRPM URLs are no longer valid, please update the > URLs. Seems like they are at https://copr-be.cloud.fedoraproject.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01469069-mlxbf-bootctl/ Spencer, can you confirm? Spencer, Can you confirm so we can get this moved on. Thank you. Stalled review, resetting ticket status This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it. The needinfo request[s] on this closed bug have been removed as they have been unresolved for 365 days |