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 [off-list ref] 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.))

[...]

quoted

+               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).

quoted

-               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.

[...]

quoted

-                                            "Length (62.5um)", 10, "m");
+                               "Length (62.5um)", 10, "m");

These changes are unnecessary.

Agreed, removed from the patch.

Best regards,
-- 
Aurélien Guillaume