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

[Susant Sahani]

I will fix these . thanks for review.

On 04/21/2015 05:00 AM, Lennart Poettering wrote:
> 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
>

Susant