Spec URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01381236-mlxbf-bootctl/mlxbf-bootctl.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fedora-rawhide-aarch64/01381236-mlxbf-bootctl/mlxbf-bootctl-1.1-3.fc33.src.rpm Scratch Koji URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=44462386 Source Repository: https://github.com/Mellanox/mlxbf-bootctl Fedora Account System Username: slingard Description: This program controls bootloader features on Mellanox BlueField hardware, such as installing UEFI/ATF to the chip, activating watchdog timers, and which of the redundant bootloader partitions is active. Note: This is my first package, and require a sponsor.
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