[systemd-devel] [systemd-commits] man/systemd.netdev.xml src/libsystemd src/network

[Lennart Poettering]

On Mon, 20.04.15 15:27, Tom Gundersen (tomegun at kemper.freedesktop.org) wrote:

>  
> +      <varlistentry>
> +        <term><varname>LearnPacketIntvSec,=</varname></term>
> +        <listitem>
> +          <para>Specifies the number of seconds between instances where the bonding
> +          driver sends learning packets to each slaves peer switch.
> +          The valid range is 1 - 0x7fffffff; the default value is 1. This Option
> +          has effect only in balance-tlb and balance-alb modes.</para>
> +        </listitem>
> +      </varlistentry>


We usually don't abbreviate options unnecessarily. Please use
"Interval" instead of "Intv".

> +      <varlistentry>
> +        <term><varname>FailOverMac=</varname></term>
> +        <listitem>
> +          <para>Specifies whether active-backup mode should set all slaves to
> +          the same MAC address at enslavement or, when enabled, perform special handling of the
> +          bond's MAC address in accordance with the selected policy. The default policy is none.
> +          Possible values are
> +          <literal>none</literal>,
> +          <literal>active</literal>,
> +          <literal>follow</literal>
> +          </para>
> +        </listitem>
> +      </varlistentry>

The option MACAddress= is currently spelt with an uppercase MAC. In
fact most options containing acronyms use uppercase naming. hence
FailOverMAC=.

That said, shouldn't this be FailOverMACMode= or FailOverMACPolicy= or so?

> +
> +      <varlistentry>
> +        <term><varname>ArpValidate=</varname></term>
> +        <listitem>
> +          <para>Specifies whether or not ARP probes and replies should be
> +          validated in any mode that supports arp monitoring, or whether
> +          non-ARP traffic should be filtered (disregarded) for link
> +          monitoring purposes. Possible values are
> +          <literal>none</literal>,
> +          <literal>active</literal>,
> +          <literal>backup</literal>,
> +          <literal>all</literal>
> +          </para>
> +        </listitem>
> +      </varlistentry>

Uppercase "ARP" please, see above. ARPValidate=

> +      <varlistentry>
> +        <term><varname>ArpIntervalSec=</varname></term>
> +        <listitem>
> +          <para>Specifies the ARP link monitoring frequency in milliseconds.
> +          A value of 0 disables ARP monitoring. The default value is 0.
> +          </para>
> +        </listitem>
> +      </varlistentry>

Good that Interval is spelt out this time. But s/Arp/ARP/ please.

> +      <varlistentry>
> +        <term><varname>ArpIpTargets=</varname></term>
> +        <listitem>
> +          <para>Specifies the IP addresses to use as ARP monitoring peers when
> +          ArpIntervalSec is greater than 0. These are the targets of the ARP request
> +          sent to determine the health of the link to the targets.
> +          Specify these values in ipv4 dotted decimal format. At least one IP
> +          address must be given for ARP monitoring to function. The
> +          maximum number of targets that can be specified is 16. The
> +          default value is no IP addresses.
> +          </para>
> +        </listitem>
> +      </varlistentry>

s/Arp/ARP/

s/Ip/IP/

Maybe append "Address"?

> +
> +      <varlistentry>
> +        <term><varname>ArpAllTargets=</varname></term>
> +        <listitem>
> +          <para>Specifies the quantity of ArpIpTargets that must be reachable
> +          in order for the ARP monitor to consider a slave as being up.
> +          This option affects only active-backup mode for slaves with
> +          ArpValidate enabled. Possible values are
> +          <literal>any</literal>,
> +          <literal>all</literal>
> +          </para>
> +        </listitem>
> +      </varlistentry>

similar...

> +
> +      <varlistentry>
> +        <term><varname>PrimaryReselect=</varname></term>
> +        <listitem>
> +          <para>Specifies the reselection policy for the primary slave.  This
> +          affects how the primary slave is chosen to become the active slave
> +          when failure of the active slave or recovery of the primary slave
> +          occurs. This option is designed to prevent flip-flopping between
> +          the primary slave and other slaves.  Possible values are
> +          <literal>always</literal>,
> +          <literal>better</literal>,
> +          <literal>failure</literal>
> +          </para>
> +        </listitem>
> +      </varlistentry>

PrimaryReselectPolicy= or so?

> +
> +      <varlistentry>
> +        <term><varname>ResendIGMP=</varname></term>
> +        <listitem>
> +          <para>Specifies the number of IGMP membership reports to be issued after
> +          a failover event. One membership report is issued immediately after
> +          the failover, subsequent packets are sent in each 200ms interval.
> +          The valid range is (0 - 255). Defaults to 1. A value of 0
> +          prevents the IGMP membership report from being issued in response
> +          to the failover event.
> +          </para>
> +        </listitem>
> +      </varlistentry>

I like the that the "IGMP" name is upper case!

> +      <varlistentry>
> +        <term><varname>NumGratuitousARP=</varname></term>
> +        <listitem>
> +          <para>Specify the number of peer notifications (gratuitous ARPs and
> +            unsolicited IPv6 Neighbor Advertisements) to be issued after a
> +            failover event.  As soon as the link is up on the new slave
> +            a peer notification is sent on the  bonding device and each
> +            VLAN sub-device.  This is repeated at each link monitor interval
> +            (ArpIntervalSec or MIIMonitorSec, whichever is active) if the number is
> +            greater than 1. The valid range is (0 - 255). Default value is 1.
> +            These options affect only the active-backup mode
> +          </para>
> +        </listitem>
> +      </varlistentry>

"Num"? Nah, please no unnecessary abbreviations.

The last sentence lacks a full stop.

>  
> +        if (b->arp_interval != 0) {
> +                r = sd_rtnl_message_append_u32(m, IFLA_BOND_ARP_INTERVAL, b->arp_interval / USEC_PER_MSEC);
> +                if (r < 0) {
> +                        log_netdev_error(netdev,
> +                                         "Could not append IFLA_BOND_ARP_INTERVAL attribute: %s",
> +                                         strerror(-r));
> +                        return r;
> +                }
> +        }

I'd really prefer if we wouldn't add new code using strerror().

Maybe it's time to introduce log_netdev_error_errno(), that is the
combination of what log_error_errno() and log_netdev_error() do? We
already have the anem for log_network_xyz()...

Lennart

-- 
Lennart Poettering, Red Hat