[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#841536: hdparm/9.50+ds-1

Hi Niels,

thank you for the comments and upload.
I've prepared an updated package addressing the issues and uploaded it to mentors:

On Fri, Oct 21, 2016 at 7:07 PM, Niels Thykier <niels@thykier.net> wrote:
Control: tags -1 moreinfo

On Fri, 21 Oct 2016 14:46:32 +0200 Alex Mestiashvili
<mailatgoogl@gmail.com> wrote:
> Package: sponsorship-requests
> Severity: wishlist
> X-Debbugs-Cc: mailatgoogl@gmail.com
> Dear mentors,
>  [...]
> Best regards,
> Alex

Hi Alex,

Thanks for your contribution.

Have you informed upstream about the following compiler warning?
hdparm.c: In function 'process_dev':
hdparm.c:2256:2: warning: this 'if' clause does not guard...
  if (!quiet)
hdparm.c:2258:3: note: ...this statement, but the latter is misleadingly
indented as if it is guarded by the 'if'
   if (do_drive_cmd(fd, args, 0)) {

I checked it seems benign, so I am not blocking the upload on this.  The
code in question being:

        if (security_freeze) {
                __u8 args[4] = {ATA_OP_SECURITY_FREEZE_LOCK,0,0,0};
        if (!quiet)
        ^^^^^^^^^^^ (should be 1 tab further in)
                printf(" issuing security freeze command\n");
                ^^^^^^^^^^^^^^ (should be 1 tab further in as well)
                if (do_drive_cmd(fd, args, 0)) {
                        err = errno;
                        perror(" HDIO_DRIVE_CMD[...] failed");


The wrong indentation comes from the quiet_security_freeze.patch fixing the bug #512175
The indentation is fixed in the latest package.

Other minor nits:
 * Spelling error in hdparm binary "Removeable" -> "Removable"
   - NB: Upstream is inconsistent with the spelling, but seems to prefer
     the "Removable" variant.

I know about this typo, but didn't want to fix it because the fix would change the output of the program
and it might impact scripts trying to identify that flag. I'll forward this and other typos to upstream.
 * Upstream's build system hardcodes -j2 on "make all" and the packaging
   uses that without any respect to DEB_BUILD_OPTIONS
   - Given the small size and low parallel limit, I don't think it is a
     huge issue (but it would have been for other packages).

Fixed with the makefile.patch
 * The debian/hdparm.preinst file appears to be redundant (its replaced
   by the debian/hdparm.maintscript)

Thanks, fixed too.

Reply to: