Address code-review items for pull-request #2890

1. Replace strtol with unhexchar, verified with valid and invalid DUID strings.
2. Fix logging to use log_syntax instead of log_error.
3. On error reading DUID, ignore read and preserve previous state.
4. Fix man-pages to use markup, remove options not yet implemented.
5. Remove spurious header line in new files.

man/networkd.conf.xml

@@ -95,21 +95,17 @@
         <term><varname>Type=</varname></term>
         <listitem><para>The type of DUID specified in this section. The following values are
         supported:</para>
-        <para>raw : If <literal>Type=raw</literal>, then <literal>RawData=</literal> specifies
+        <variablelist>
-        the entire DUID. For e.g: <literal>RawData=00:02:00:00:ab:11:f9:2a:c2:77:29:f9:5c:00</literal>
+          <varlistentry>
+            <term><option>raw</option> </term>
+            <listitem><para>If <literal>Type=raw</literal>, then <literal>RawData=</literal> specifies the
+            entire DUID. For example, <literal>RawData=00:02:00:00:ab:11:f9:2a:c2:77:29:f9:5c:00</literal>
             specifies a 14 byte long DUID-EN ("00:02"), with enterprise number 43793 ("00:00:ab:11"),
             and identifier value "f9:2a:c2:77:29:f9:5c:00".</para><para>If Type is not specified and
-        RawData is specified, Type defaults to 'raw'.</para>
+            RawData is specified, Type defaults to 'raw'.</para></listitem>
-        <para>Type will support the following values in the future:</para>
+          </varlistentry>
-        <para>link-layer-and-time : If <literal>Type=link-layer-and-time</literal>, then
+        </variablelist>
-        <literal>MACAddress=</literal> and <literal>TimeStamp=</literal> specify the hardware
+        </listitem>
-        address and time-stamp for DUID-LLT.</para>
-        <para>vendor : If <literal>Type=vendor</literal>, then <literal>EnterpriseNumber=</literal>
-        and <literal>RawData=</literal> specify the enterprise number and identifier for DUID-EN.</para>
-        <para>link-layer : If <literal>Type=link-layer</literal>, then <literal>MACAddress=</literal>
-        specifies the hardware address for DUID-LL.</para>
-        <para>uuid : If <literal>Type=uuid</literal>, then <literal>UUID=</literal> specifies DUID-UUID.
-        </para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -120,33 +116,6 @@
     </variablelist>
   </refsect1>
 
-  <refsect1>
-    <para>The following options will be supported in the future:
-    </para>
-    <variablelist>
-      <varlistentry>
-        <term><varname>MACAddress=</varname></term>
-        <listitem><para>Specifies the link-layer address for DUID Type <option>link-layer
-        </option> or <option>link-layer-and-time</option>.</para></listitem>
-      </varlistentry>
-      <varlistentry>
-        <term><varname>TimeStamp=</varname></term>
-        <listitem><para>Specifies the DUID generation time for DUID Type <option>
-        link-layer-and-time</option>.</para></listitem>
-      </varlistentry>
-      <varlistentry>
-        <term><varname>EnterpriseNumber=</varname></term>
-        <listitem><para>Specifies the enterprise number for DUID Type
-        <option>vendor</option>.</para></listitem>
-      </varlistentry>
-      <varlistentry>
-        <term><varname>UUID=</varname></term>
-        <listitem><para>Specifies the UUID for DUID Type <option>uuid</option>.</para>
-        </listitem>
-      </varlistentry>
-    </variablelist>
-  </refsect1>
-
   <refsect1>
       <title>See Also</title>
       <para>

man/systemd.network.xml

@@ -856,21 +856,17 @@
         <term><varname>Type=</varname></term>
         <listitem><para>The type of DUID specified in this section. The following values are
         supported:</para>
-        <para>raw : If <literal>Type=raw</literal>, then <literal>RawData=</literal> specifies
+        <variablelist>
-        the entire DUID. For e.g: <literal>RawData=00:02:00:00:ab:11:f9:2a:c2:77:29:f9:5c:00</literal>
+          <varlistentry>
+            <term><option>raw</option> </term>
+            <listitem><para>If <literal>Type=raw</literal>, then <literal>RawData=</literal> specifies the
+            entire DUID. For example, <literal>RawData=00:02:00:00:ab:11:f9:2a:c2:77:29:f9:5c:00</literal>
             specifies a 14 byte long DUID-EN ("00:02"), with enterprise number 43793 ("00:00:ab:11"),
             and identifier value "f9:2a:c2:77:29:f9:5c:00".</para><para>If Type is not specified and
-        RawData is specified, Type defaults to 'raw'.</para>
+            RawData is specified, Type defaults to 'raw'.</para></listitem>
-        <para>Type will support the following values in the future:</para>
+          </varlistentry>
-        <para>link-layer-and-time : If <literal>Type=link-layer-and-time</literal>, then
+        </variablelist>
-        <literal>MACAddress=</literal> and <literal>TimeStamp=</literal> specify the hardware
+        </listitem>
-        address and time-stamp for DUID-LLT.</para>
-        <para>vendor : If <literal>Type=vendor</literal>, then <literal>EnterpriseNumber=</literal>
-        and <literal>RawData=</literal> specify the enterprise number and identifier for DUID-EN.</para>
-        <para>link-layer : If <literal>Type=link-layer</literal>, then <literal>MACAddress=</literal>
-        specifies the hardware address for DUID-LL.</para>
-        <para>uuid : If <literal>Type=uuid</literal>, then <literal>UUID=</literal> specifies DUID-UUID.
-        </para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -882,33 +878,6 @@
     </variablelist>
   </refsect1>
 
-  <refsect1>
-    <para>The following options will be supported in the future:
-    </para>
-    <variablelist>
-      <varlistentry>
-        <term><varname>MACAddress=</varname></term>
-        <listitem><para>Specifies the link-layer address for DUID Type <option>link-layer
-        </option> or <option>link-layer-and-time</option>.</para></listitem>
-      </varlistentry>
-      <varlistentry>
-        <term><varname>TimeStamp=</varname></term>
-        <listitem><para>Specifies the DUID generation time for DUID Type <option>
-        link-layer-and-time</option>.</para></listitem>
-      </varlistentry>
-      <varlistentry>
-        <term><varname>EnterpriseNumber=</varname></term>
-        <listitem><para>Specifies the enterprise number for DUID Type <option>
-        vendor</option>.</para></listitem>
-      </varlistentry>
-      <varlistentry>
-        <term><varname>UUID=</varname></term>
-        <listitem><para>Specifies the UUID for DUID Type <option>uuid</option>.</para>
-        </listitem>
-      </varlistentry>
-    </variablelist>
-  </refsect1>
-
   <refsect1>
     <title>[DHCPServer] Section Options</title>
     <para>The <literal>[DHCPServer]</literal> section contains

src/libsystemd-network/network-internal.c

@@ -355,8 +355,9 @@ int config_parse_iaid(const char *unit,
 
         r = safe_atou32(rvalue, &iaid);
         if (r < 0) {
-                log_syntax(unit, LOG_ERR, filename, line, 0, "Unable to read IAID: %s", rvalue);
+                log_syntax(unit, LOG_ERR, filename, line, r,
-                return r;
+                           "Unable to read IAID, ignoring assignment: %s", rvalue);
+                return 0;
         }
 
         *((uint32_t *)data) = iaid;

src/network/networkd-conf.c

@@ -1,5 +1,3 @@
-/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
-
 /***
   This file is part of systemd.
 
@@ -24,6 +22,7 @@
 #include "conf-parser.h"
 #include "def.h"
 #include "dhcp-identifier.h"
+#include "hexdecoct.h"
 #include "networkd-conf.h"
 #include "string-table.h"
 
@@ -58,53 +57,39 @@ int config_parse_duid_rawdata(
                 const char *rvalue,
                 void *data,
                 void *userdata) {
-        int r;
+        int r, n1, n2, byte;
-        long byte;
+        char *cbyte;
-        char *cbyte, *pnext;
         const char *pduid = rvalue;
-        size_t count = 0, duid_index = 0;
+        Manager *m = userdata;
-        Manager *m;
+        Network *n = userdata;
-        Network *n;
+        DUIDType duidtype;
-        DUIDType *duid_type;
+        uint16_t dhcp_duid_type = 0;
-        uint16_t *dhcp_duid_type;
+        uint8_t dhcp_duid[MAX_DUID_LEN];
-        size_t *dhcp_duid_len;
+        size_t len, count = 0, duid_start_offset = 0, dhcp_duid_len = 0;
-        uint8_t *dhcp_duid;
 
         assert(filename);
         assert(lvalue);
         assert(rvalue);
         assert(userdata);
 
-        if (ltype == DUID_CONFIG_SOURCE_GLOBAL) {
+        duidtype = (ltype == DUID_CONFIG_SOURCE_GLOBAL) ? m->duid_type
-                m = userdata;
+                                                        : n->duid_type;
-                duid_type = &m->duid_type;
-                dhcp_duid_type = &m->dhcp_duid_type;
-                dhcp_duid_len = &m->dhcp_duid_len;
-                dhcp_duid = m->dhcp_duid;
-        } else {
-                /* DUID_CONFIG_SOURCE_NETWORK */
-                n = userdata;
-                duid_type = &n->duid_type;
-                dhcp_duid_type = &n->dhcp_duid_type;
-                dhcp_duid_len = &n->dhcp_duid_len;
-                dhcp_duid = n->dhcp_duid;
-        }
 
-        if (*duid_type == _DUID_TYPE_INVALID)
+        if (duidtype == _DUID_TYPE_INVALID)
-                *duid_type = DUID_TYPE_RAW;
+                duidtype = DUID_TYPE_RAW;
 
-        switch (*duid_type) {
+        switch (duidtype) {
         case DUID_TYPE_LLT:
                 /* RawData contains DUID-LLT link-layer address (offset 6) */
-                duid_index = 6;
+                duid_start_offset = 6;
                 break;
         case DUID_TYPE_EN:
                 /* RawData contains DUID-EN identifier (offset 4) */
-                duid_index = 4;
+                duid_start_offset = 4;
                 break;
         case DUID_TYPE_LL:
                 /* RawData contains DUID-LL link-layer address (offset 2) */
-                duid_index = 2;
+                duid_start_offset = 2;
                 break;
         case DUID_TYPE_UUID:
                 /* RawData specifies UUID (offset 0) - fall thru */
@@ -114,42 +99,66 @@ int config_parse_duid_rawdata(
                 break;
         }
 
-        if (*duid_type != DUID_TYPE_RAW)
+        if (duidtype != DUID_TYPE_RAW)
-                *dhcp_duid_type = (uint16_t)(*duid_type);
+                dhcp_duid_type = (uint16_t)duidtype;
 
         /* RawData contains DUID in format " NN:NN:NN... " */
-        while (true) {
+        for (;;) {
                 r = extract_first_word(&pduid, &cbyte, ":", 0);
                 if (r < 0) {
-                        log_error("Failed to read DUID.");
+                        log_syntax(unit, LOG_ERR, filename, line, r,
-                        return -EINVAL;
+                                   "Failed to read DUID, ignoring assignment: %s.", rvalue);
+                        goto exit;
                 }
                 if (r == 0)
                         break;
-                if (duid_index >= MAX_DUID_LEN) {
+                if ((duid_start_offset + dhcp_duid_len) >= MAX_DUID_LEN) {
-                        log_error("DUID length exceeds maximum length.");
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
-                        return -EINVAL;
+                                   "Max DUID length exceeded, ignoring assignment: %s.", rvalue);
+                        goto exit;
                 }
 
-                errno = 0;
+                len = strlen(cbyte);
-                byte = strtol(cbyte, &pnext, 16);
+                if ((len == 0) || (len > 2)) {
-                if ((errno == ERANGE && (byte == LONG_MAX || byte == LONG_MIN))
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
-                    || (errno != 0 && byte == 0) || (cbyte == pnext)) {
+                                   "Invalid length - DUID byte: %s, ignoring assignment: %s.", cbyte, rvalue);
-                        log_error("Invalid DUID byte: %s.", cbyte);
+                        goto exit;
-                        return -EINVAL; 
                 }
+                n2 = 0;
+                n1 = unhexchar(cbyte[0]);
+                if (len == 2)
+                        n2 = unhexchar(cbyte[1]);
+                if ((n1 < 0) || (n2 < 0)) {
+                        log_syntax(unit, LOG_ERR, filename, line, 0,
+                                   "Invalid DUID byte: %s. Ignoring assignment: %s.", cbyte, rvalue);
+                        goto exit;
+                }
+                byte = (n1 << (4 * (len-1))) | n2;
 
                 /* If DUID_TYPE_RAW, first two bytes hold DHCP DUID type code */
-                if ((*duid_type == DUID_TYPE_RAW) && (count < 2)) {
+                if ((duidtype == DUID_TYPE_RAW) && (count < 2)) {
-                        *dhcp_duid_type |= (byte << (8 * (1 - count)));
+                        dhcp_duid_type |= (byte << (8 * (1 - count)));
                         count++;
                         continue;
                 }
 
-                dhcp_duid[duid_index++] = byte;
+                dhcp_duid[duid_start_offset + dhcp_duid_len] = byte;
+                dhcp_duid_len++;
         }
 
-        *dhcp_duid_len = duid_index;
+        if (ltype == DUID_CONFIG_SOURCE_GLOBAL) {
+                m->duid_type = duidtype;
+                m->dhcp_duid_type = dhcp_duid_type;
+                m->dhcp_duid_len = dhcp_duid_len;
+                memcpy(&m->dhcp_duid[duid_start_offset], dhcp_duid, dhcp_duid_len);
+        } else {
+                /* DUID_CONFIG_SOURCE_NETWORK */
+                n->duid_type = duidtype;
+                n->dhcp_duid_type = dhcp_duid_type;
+                n->dhcp_duid_len = dhcp_duid_len;
+                memcpy(&n->dhcp_duid[duid_start_offset], dhcp_duid, dhcp_duid_len);
+        }
 
+exit:
         return 0;
 }

src/network/networkd-conf.h

@@ -1,5 +1,3 @@
-/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
-
 #pragma once
 
 /***