netdev - Re: Optics (SFP) monitoring on ixgbe and igbe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 2 Dec 2012 22:47:09 +0100
From:	Aurélien <footplus@...il.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: Re: Optics (SFP) monitoring on ixgbe and igbe

Hi Ben,

Thanks for your review. Here's a fixed-up version, according to your remarks.

On Sat, Dec 1, 2012 at 5:18 AM, Ben Hutchings <bhutchings@...arflare.com> wrote:
>
> This version drops the -lm completely, so it doesn't link.  Maybe you
> edited the generated Makefile or Makefile.in?

No, I just stupidly forgot to make distclean & autogen after removing
all libm checks. Re-added AC_CHECK_LIB to link with it.

>
> The option alias should be included in the manual page and in a
> (trivial) test case in test-cmdline.c.
>

Included in the man page, along with a modified option description,
and a test equivalent to --dump-module-eeprom, which passes.

>
> The indentation is still weird, though:
>
> [...]
>
> These comments should be lined up vertically.

Yup, I had a mixup between tab/whitespaces, and my Vim config did not
help. Fixed.

>
> be32toh() is non-standard and was apparently added to glibc relatively
> recently (version 2.9).  Therefore please use the equivalent ntohl()
> instead.

Did that, it indeed works just fine. I should have used that from the start.

>
> Function-like macros generally shouldn't be defined with a trailing
> semi-colon, as that will be added at the point of use.
>

That was a copy/paste typo, fixed.

> The backslashes should be lined up on the right, and continuation lines
> within parentheses should be indented so they begin just to the right of
> the opening parenthesis, e.g.:
>
> #define PRINT_VCC(string, index)                                \
>         printf("\t%-41s : %.4f V\n", (string),                  \
>                (double)(sd.sfp_voltage[(index)] / 10000.))
>
> [...]
>> +               PRINT_xX_PWR("Laser output power low warning threshold",
>> +                                        sd.tx_power, LWARN);
>
> The continuation lines are over-indented here.
>

Fixed (was using wrong tabstop width at 4).

>> -               printf("\tActive Cu cmplnce.  : 0x%02x", id[60]);
>> +               printf("\t%-41s : 0x%02x", "Active copper compliance", id[60]);
>
> If you want to change these labels, do that in a separate patch.
>

There's no real need, so I'll leave those alone, just changing the
alignment like for the other labels.

> [...]
>> -                                            "Length (62.5um)", 10, "m");
>> +                               "Length (62.5um)", 10, "m");
>
> These changes are unnecessary.
>

Agreed, removed from the patch.

Best regards,
-- 
Aurélien Guillaume

Download attachment "0001-Implemented-basic-optics-diagnostics-for-SFF-8472.patch" of type "application/octet-stream" (22638 bytes)