diff mbox

[ovs-dev,v2,02/21] smap: New function smap_get_ullong().

Message ID 1470672872-19450-3-git-send-email-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff Aug. 8, 2016, 4:14 p.m. UTC
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/netdev-dpdk.c  |  16 ++----
 lib/netdev-linux.c |  89 +++++++++--------------------
 lib/smap.c         |  11 ++++
 lib/smap.h         |   2 +
 vswitchd/bridge.c  | 162 +++++++++++++++++------------------------------------
 5 files changed, 92 insertions(+), 188 deletions(-)

Comments

Ryan Moats Aug. 8, 2016, 4:49 p.m. UTC | #1
"dev" <dev-bounces@openvswitch.org> wrote on 08/08/2016 11:14:13 AM:

> From: Ben Pfaff <blp@ovn.org>
> To: dev@openvswitch.org
> Cc: Ben Pfaff <blp@ovn.org>
> Date: 08/08/2016 11:15 AM
> Subject: [ovs-dev] [PATCH v2 02/21] smap: New function smap_get_ullong().
> Sent by: "dev" <dev-bounces@openvswitch.org>
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---

Looks sane enough, no compiling or test surprises, so...

Acked-by: Ryan Moats <rmoats@us.ibm.com>
diff mbox

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index f37ec1c..bf3a898 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2763,18 +2763,14 @@  egress_policer_qos_construct(struct netdev *netdev,
 {
     struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
     struct egress_policer *policer;
-    const char *cir_s;
-    const char *cbs_s;
     int err = 0;
 
     rte_spinlock_lock(&dev->qos_lock);
     policer = xmalloc(sizeof *policer);
     qos_conf_init(&policer->qos_conf, &egress_policer_ops);
     dev->qos_conf = &policer->qos_conf;
-    cir_s = smap_get(details, "cir");
-    cbs_s = smap_get(details, "cbs");
-    policer->app_srtcm_params.cir = cir_s ? strtoull(cir_s, NULL, 10) : 0;
-    policer->app_srtcm_params.cbs = cbs_s ? strtoull(cbs_s, NULL, 10) : 0;
+    policer->app_srtcm_params.cir = smap_get_ullong(details, "cir", 0);
+    policer->app_srtcm_params.cbs = smap_get_ullong(details, "cbs", 0);
     policer->app_srtcm_params.ebs = 0;
     err = rte_meter_srtcm_config(&policer->egress_meter,
                                     &policer->app_srtcm_params);
@@ -2808,15 +2804,11 @@  static int
 egress_policer_qos_set(struct netdev *netdev, const struct smap *details)
 {
     struct egress_policer *policer;
-    const char *cir_s;
-    const char *cbs_s;
     int err = 0;
 
     policer = egress_policer_get__(netdev);
-    cir_s = smap_get(details, "cir");
-    cbs_s = smap_get(details, "cbs");
-    policer->app_srtcm_params.cir = cir_s ? strtoull(cir_s, NULL, 10) : 0;
-    policer->app_srtcm_params.cbs = cbs_s ? strtoull(cbs_s, NULL, 10) : 0;
+    policer->app_srtcm_params.cir = smap_get_ullong(details, "cir", 0);
+    policer->app_srtcm_params.cbs = smap_get_ullong(details, "cbs", 0);
     policer->app_srtcm_params.ebs = 0;
     err = rte_meter_srtcm_config(&policer->egress_meter,
                                     &policer->app_srtcm_params);
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index fa37bcf..41173d2 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1,5 +1,5 @@ 
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -2937,17 +2937,9 @@  static void
 codel_parse_qdisc_details__(struct netdev *netdev OVS_UNUSED,
                             const struct smap *details, struct codel *codel)
 {
-    const char *target_s;
-    const char *limit_s;
-    const char *interval_s;
-
-    target_s = smap_get(details, "target");
-    limit_s = smap_get(details, "limit");
-    interval_s = smap_get(details, "interval");
-
-    codel->target = target_s ? strtoull(target_s, NULL, 10) : 0;
-    codel->limit = limit_s ? strtoull(limit_s, NULL, 10) : 0;
-    codel->interval = interval_s ? strtoull(interval_s, NULL, 10) : 0;
+    codel->target = smap_get_ullong(details, "target", 0);
+    codel->limit = smap_get_ullong(details, "limit", 0);
+    codel->interval = smap_get_ullong(details, "interval", 0);
 
     if (!codel->target) {
         codel->target = 5000;
@@ -3168,22 +3160,12 @@  static void
 fqcodel_parse_qdisc_details__(struct netdev *netdev OVS_UNUSED,
                           const struct smap *details, struct fqcodel *fqcodel)
 {
-    const char *target_s;
-    const char *limit_s;
-    const char *interval_s;
-    const char *flows_s;
-    const char *quantum_s;
-
-    target_s = smap_get(details, "target");
-    limit_s = smap_get(details, "limit");
-    interval_s = smap_get(details, "interval");
-    flows_s = smap_get(details, "flows");
-    quantum_s = smap_get(details, "quantum");
-    fqcodel->target = target_s ? strtoull(target_s, NULL, 10) : 0;
-    fqcodel->limit = limit_s ? strtoull(limit_s, NULL, 10) : 0;
-    fqcodel->interval = interval_s ? strtoull(interval_s, NULL, 10) : 0;
-    fqcodel->flows = flows_s ? strtoull(flows_s, NULL, 10) : 0;
-    fqcodel->quantum = quantum_s ? strtoull(quantum_s, NULL, 10) : 0;
+    fqcodel->target = smap_get_ullong(details, "target", 0);
+    fqcodel->limit = smap_get_ullong(details, "limit", 0);
+    fqcodel->interval = smap_get_ullong(details, "interval", 0);
+    fqcodel->flows = smap_get_ullong(details, "flows", 0);
+    fqcodel->quantum = smap_get_ullong(details, "quantum", 0);
+
     if (!fqcodel->target) {
         fqcodel->target = 5000;
     }
@@ -3404,27 +3386,20 @@  static void
 sfq_parse_qdisc_details__(struct netdev *netdev,
                           const struct smap *details, struct sfq *sfq)
 {
-    const char *perturb_s;
-    const char *quantum_s;
-    int mtu;
-    int mtu_error;
+    sfq->perturb = smap_get_ullong(details, "perturb", 0);
+    sfq->quantum = smap_get_ullong(details, "quantum", 0);
 
-    perturb_s = smap_get(details, "perturb");
-    quantum_s = smap_get(details, "quantum");
-    sfq->perturb = perturb_s ? strtoull(perturb_s, NULL, 10) : 0;
-    sfq->quantum = quantum_s ? strtoull(quantum_s, NULL, 10) : 0;
     if (!sfq->perturb) {
         sfq->perturb = 10;
     }
 
     if (!sfq->quantum) {
-        mtu_error = netdev_linux_get_mtu__(netdev_linux_cast(netdev), &mtu);
-        if (!mtu_error) {
+        int mtu;
+        if (!netdev_linux_get_mtu__(netdev_linux_cast(netdev), &mtu)) {
             sfq->quantum = mtu;
         } else {
             VLOG_WARN_RL(&rl, "when using SFQ, you must specify quantum on a "
                          "device without mtu");
-            return;
         }
     }
 }
@@ -3697,10 +3672,8 @@  htb_parse_qdisc_details__(struct netdev *netdev_,
                           const struct smap *details, struct htb_class *hc)
 {
     struct netdev_linux *netdev = netdev_linux_cast(netdev_);
-    const char *max_rate_s;
 
-    max_rate_s = smap_get(details, "max-rate");
-    hc->max_rate = max_rate_s ? strtoull(max_rate_s, NULL, 10) / 8 : 0;
+    hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8;
     if (!hc->max_rate) {
         enum netdev_features current;
 
@@ -3718,10 +3691,6 @@  htb_parse_class_details__(struct netdev *netdev,
                           const struct smap *details, struct htb_class *hc)
 {
     const struct htb *htb = htb_get__(netdev);
-    const char *min_rate_s = smap_get(details, "min-rate");
-    const char *max_rate_s = smap_get(details, "max-rate");
-    const char *burst_s = smap_get(details, "burst");
-    const char *priority_s = smap_get(details, "priority");
     int mtu, error;
 
     error = netdev_linux_get_mtu__(netdev_linux_cast(netdev), &mtu);
@@ -3733,14 +3702,15 @@  htb_parse_class_details__(struct netdev *netdev,
 
     /* HTB requires at least an mtu sized min-rate to send any traffic even
      * on uncongested links. */
-    hc->min_rate = min_rate_s ? strtoull(min_rate_s, NULL, 10) / 8 : 0;
+    hc->min_rate = smap_get_ullong(details, "min-rate", 0) / 8;
     hc->min_rate = MAX(hc->min_rate, mtu);
     hc->min_rate = MIN(hc->min_rate, htb->max_rate);
 
     /* max-rate */
-    hc->max_rate = (max_rate_s
-                    ? strtoull(max_rate_s, NULL, 10) / 8
-                    : htb->max_rate);
+    hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8;
+    if (!hc->max_rate) {
+        hc->max_rate = htb->max_rate;
+    }
     hc->max_rate = MAX(hc->max_rate, hc->min_rate);
     hc->max_rate = MIN(hc->max_rate, htb->max_rate);
 
@@ -3753,11 +3723,11 @@  htb_parse_class_details__(struct netdev *netdev,
      * doesn't include the Ethernet header, we need to add at least 14 (18?) to
      * the MTU.  We actually add 64, instead of 14, as a guard against
      * additional headers get tacked on somewhere that we're not aware of. */
-    hc->burst = burst_s ? strtoull(burst_s, NULL, 10) / 8 : 0;
+    hc->burst = smap_get_ullong(details, "burst", 0) / 8;
     hc->burst = MAX(hc->burst, mtu + 64);
 
     /* priority */
-    hc->priority = priority_s ? strtoul(priority_s, NULL, 10) : 0;
+    hc->priority = smap_get_ullong(details, "priority", 0);
 
     return 0;
 }
@@ -4175,12 +4145,8 @@  hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap *details,
                            struct hfsc_class *class)
 {
     struct netdev_linux *netdev = netdev_linux_cast(netdev_);
-    uint32_t max_rate;
-    const char *max_rate_s;
-
-    max_rate_s = smap_get(details, "max-rate");
-    max_rate   = max_rate_s ? strtoull(max_rate_s, NULL, 10) / 8 : 0;
 
+    uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8;
     if (!max_rate) {
         enum netdev_features current;
 
@@ -4200,19 +4166,14 @@  hfsc_parse_class_details__(struct netdev *netdev,
 {
     const struct hfsc *hfsc;
     uint32_t min_rate, max_rate;
-    const char *min_rate_s, *max_rate_s;
 
     hfsc       = hfsc_get__(netdev);
-    min_rate_s = smap_get(details, "min-rate");
-    max_rate_s = smap_get(details, "max-rate");
 
-    min_rate = min_rate_s ? strtoull(min_rate_s, NULL, 10) / 8 : 0;
+    min_rate = smap_get_ullong(details, "min-rate", 0) / 8;
     min_rate = MAX(min_rate, 1);
     min_rate = MIN(min_rate, hfsc->max_rate);
 
-    max_rate = (max_rate_s
-                ? strtoull(max_rate_s, NULL, 10) / 8
-                : hfsc->max_rate);
+    max_rate = smap_get_ullong(details, "max-rate", hfsc->max_rate * 8) / 8;
     max_rate = MAX(max_rate, min_rate);
     max_rate = MIN(max_rate, hfsc->max_rate);
 
diff --git a/lib/smap.c b/lib/smap.c
index 7b35da6..0a5e54a 100644
--- a/lib/smap.c
+++ b/lib/smap.c
@@ -231,6 +231,17 @@  smap_get_int(const struct smap *smap, const char *key, int def)
     return value ? atoi(value) : def;
 }
 
+/* Gets the value associated with 'key' in 'smap' and converts it to an int
+ * using strtoull().  If 'key' is not in 'smap', returns 'def'. */
+unsigned long long int
+smap_get_ullong(const struct smap *smap, const char *key,
+                unsigned long long def)
+{
+    const char *value = smap_get(smap, key);
+
+    return value ? strtoull(value, NULL, 10) : def;
+}
+
 /* Gets the value associated with 'key' in 'smap' and converts it to a UUID
  * using uuid_from_string().  Returns true if successful, false if 'key' is not
  * in 'smap' or if 'key' does not have the correct syntax for a UUID. */
diff --git a/lib/smap.h b/lib/smap.h
index 9ad71e7..49d31cb 100644
--- a/lib/smap.h
+++ b/lib/smap.h
@@ -88,6 +88,8 @@  const char *smap_get_def(const struct smap *, const char *key,
 struct smap_node *smap_get_node(const struct smap *, const char *);
 bool smap_get_bool(const struct smap *smap, const char *key, bool def);
 int smap_get_int(const struct smap *smap, const char *key, int def);
+unsigned long long int smap_get_ullong(const struct smap *, const char *key,
+                                       unsigned long long def);
 bool smap_get_uuid(const struct smap *, const char *key, struct uuid *);
 
 bool smap_is_empty(const struct smap *);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index ddf1fe5..d8fc387 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1430,12 +1430,9 @@  port_configure_rstp(const struct ofproto *ofproto, struct port *port,
         port_s->priority = RSTP_DEFAULT_PORT_PRIORITY;
     }
 
-    config_str = smap_get(&port->cfg->other_config, "rstp-admin-p2p-mac");
-    if (config_str) {
-        port_s->admin_p2p_mac_state = strtoul(config_str, NULL, 0);
-    } else {
-        port_s->admin_p2p_mac_state = RSTP_ADMIN_P2P_MAC_FORCE_TRUE;
-    }
+    port_s->admin_p2p_mac_state = smap_get_ullong(
+        &port->cfg->other_config, "rstp-admin-p2p-mac",
+        RSTP_ADMIN_P2P_MAC_FORCE_TRUE);
 
     port_s->admin_port_state = smap_get_bool(&port->cfg->other_config,
                                              "rstp-admin-port-state", true);
@@ -1476,33 +1473,17 @@  bridge_configure_stp(struct bridge *br, bool enable_stp)
             br_s.system_id = eth_addr_to_uint64(br->ea);
         }
 
-        config_str = smap_get(&br->cfg->other_config, "stp-priority");
-        if (config_str) {
-            br_s.priority = strtoul(config_str, NULL, 0);
-        } else {
-            br_s.priority = STP_DEFAULT_BRIDGE_PRIORITY;
-        }
+        br_s.priority = smap_get_ullong(&br->cfg->other_config, "stp-priority",
+                                        STP_DEFAULT_BRIDGE_PRIORITY);
+        br_s.hello_time = smap_get_ullong(&br->cfg->other_config,
+                                          "stp-hello-time",
+                                          STP_DEFAULT_HELLO_TIME);
 
-        config_str = smap_get(&br->cfg->other_config, "stp-hello-time");
-        if (config_str) {
-            br_s.hello_time = strtoul(config_str, NULL, 10) * 1000;
-        } else {
-            br_s.hello_time = STP_DEFAULT_HELLO_TIME;
-        }
-
-        config_str = smap_get(&br->cfg->other_config, "stp-max-age");
-        if (config_str) {
-            br_s.max_age = strtoul(config_str, NULL, 10) * 1000;
-        } else {
-            br_s.max_age = STP_DEFAULT_MAX_AGE;
-        }
-
-        config_str = smap_get(&br->cfg->other_config, "stp-forward-delay");
-        if (config_str) {
-            br_s.fwd_delay = strtoul(config_str, NULL, 10) * 1000;
-        } else {
-            br_s.fwd_delay = STP_DEFAULT_FWD_DELAY;
-        }
+        br_s.max_age = smap_get_ullong(&br->cfg->other_config, "stp-max-age",
+                                       STP_DEFAULT_HELLO_TIME / 1000) * 1000;
+        br_s.fwd_delay = smap_get_ullong(&br->cfg->other_config,
+                                         "stp-forward-delay",
+                                         STP_DEFAULT_FWD_DELAY / 1000) * 1000;
 
         /* Configure STP on the bridge. */
         if (ofproto_set_stp(br->ofproto, &br_s)) {
@@ -1571,49 +1552,19 @@  bridge_configure_rstp(struct bridge *br, bool enable_rstp)
             br_s.address = eth_addr_to_uint64(br->ea);
         }
 
-        config_str = smap_get(&br->cfg->other_config, "rstp-priority");
-        if (config_str) {
-            br_s.priority = strtoul(config_str, NULL, 0);
-        } else {
-            br_s.priority = RSTP_DEFAULT_PRIORITY;
-        }
-
-        config_str = smap_get(&br->cfg->other_config, "rstp-ageing-time");
-        if (config_str) {
-            br_s.ageing_time = strtoul(config_str, NULL, 0);
-        } else {
-            br_s.ageing_time = RSTP_DEFAULT_AGEING_TIME;
-        }
-
-        config_str = smap_get(&br->cfg->other_config,
-                              "rstp-force-protocol-version");
-        if (config_str) {
-            br_s.force_protocol_version = strtoul(config_str, NULL, 0);
-        } else {
-            br_s.force_protocol_version = FPV_DEFAULT;
-        }
-
-        config_str = smap_get(&br->cfg->other_config, "rstp-max-age");
-        if (config_str) {
-            br_s.bridge_max_age = strtoul(config_str, NULL, 10);
-        } else {
-            br_s.bridge_max_age = RSTP_DEFAULT_BRIDGE_MAX_AGE;
-        }
-
-        config_str = smap_get(&br->cfg->other_config, "rstp-forward-delay");
-        if (config_str) {
-            br_s.bridge_forward_delay = strtoul(config_str, NULL, 10);
-        } else {
-            br_s.bridge_forward_delay = RSTP_DEFAULT_BRIDGE_FORWARD_DELAY;
-        }
-
-        config_str = smap_get(&br->cfg->other_config,
-                              "rstp-transmit-hold-count");
-        if (config_str) {
-            br_s.transmit_hold_count = strtoul(config_str, NULL, 10);
-        } else {
-            br_s.transmit_hold_count = RSTP_DEFAULT_TRANSMIT_HOLD_COUNT;
-        }
+        const struct smap *oc = &br->cfg->other_config;
+        br_s.priority = smap_get_ullong(oc, "rstp-priority",
+                                        RSTP_DEFAULT_PRIORITY);
+        br_s.ageing_time = smap_get_ullong(oc, "rstp-ageing-time",
+                                           RSTP_DEFAULT_AGEING_TIME);
+        br_s.force_protocol_version = smap_get_ullong(
+            oc, "rstp-force-protocol-version", FPV_DEFAULT);
+        br_s.bridge_max_age = smap_get_ullong(oc, "rstp-max-age",
+                                              RSTP_DEFAULT_BRIDGE_MAX_AGE);
+        br_s.bridge_forward_delay = smap_get_ullong(
+            oc, "rstp-forward-delay", RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
+        br_s.transmit_hold_count = smap_get_ullong(
+            oc, "rstp-transmit-hold-count", RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 
         /* Configure RSTP on the bridge. */
         if (ofproto_set_rstp(br->ofproto, &br_s)) {
@@ -1876,21 +1827,16 @@  bridge_configure_forward_bpdu(struct bridge *br)
 static void
 bridge_configure_mac_table(struct bridge *br)
 {
-    const char *idle_time_str;
-    int idle_time;
-
-    const char *mac_table_size_str;
-    int mac_table_size;
-
-    idle_time_str = smap_get(&br->cfg->other_config, "mac-aging-time");
-    idle_time = (idle_time_str && atoi(idle_time_str)
-                 ? atoi(idle_time_str)
-                 : MAC_ENTRY_DEFAULT_IDLE_TIME);
+    const struct smap *oc = &br->cfg->other_config;
+    int idle_time = smap_get_int(oc, "mac-aging-time", 0);
+    if (!idle_time) {
+        idle_time = MAC_ENTRY_DEFAULT_IDLE_TIME;
+    }
 
-    mac_table_size_str = smap_get(&br->cfg->other_config, "mac-table-size");
-    mac_table_size = (mac_table_size_str && atoi(mac_table_size_str)
-                      ? atoi(mac_table_size_str)
-                      : MAC_DEFAULT_MAX);
+    int mac_table_size = smap_get_int(oc, "mac-table-size", 0);
+    if (!mac_table_size) {
+        mac_table_size = MAC_DEFAULT_MAX;
+    }
 
     ofproto_set_mac_table_config(br->ofproto, idle_time, mac_table_size);
 }
@@ -1904,24 +1850,17 @@  bridge_configure_mcast_snooping(struct bridge *br)
     } else {
         struct port *port;
         struct ofproto_mcast_snooping_settings br_s;
-        const char *idle_time_str;
-        const char *max_entries_str;
-
-        idle_time_str = smap_get(&br->cfg->other_config,
-                                 "mcast-snooping-aging-time");
-        br_s.idle_time = (idle_time_str && atoi(idle_time_str)
-                          ? atoi(idle_time_str)
-                          : MCAST_ENTRY_DEFAULT_IDLE_TIME);
-
-        max_entries_str = smap_get(&br->cfg->other_config,
-                                   "mcast-snooping-table-size");
-        br_s.max_entries = (max_entries_str && atoi(max_entries_str)
-                            ? atoi(max_entries_str)
+
+        const struct smap *oc = &br->cfg->other_config;
+        int idle_time = smap_get_int(oc, "mcast-snooping-aging-time", 0);
+        br_s.idle_time = idle_time ? idle_time : MCAST_ENTRY_DEFAULT_IDLE_TIME;
+        int max_entries = smap_get_int(oc, "mcast-snooping-table-size", 0);
+        br_s.max_entries = (max_entries
+                            ? max_entries
                             : MCAST_DEFAULT_MAX_ENTRIES);
 
-        br_s.flood_unreg = !smap_get_bool(&br->cfg->other_config,
-                                    "mcast-snooping-disable-flood-unregistered",
-                                    false);
+        br_s.flood_unreg = !smap_get_bool(
+            oc, "mcast-snooping-disable-flood-unregistered", false);
 
         /* Configure multicast snooping on the bridge */
         if (ofproto_set_mcast_snooping(br->ofproto, &br_s)) {
@@ -2055,12 +1994,11 @@  static void
 bridge_pick_local_hw_addr(struct bridge *br, struct eth_addr *ea,
                           struct iface **hw_addr_iface)
 {
-    const char *hwaddr;
     *hw_addr_iface = NULL;
 
     /* Did the user request a particular MAC? */
-    hwaddr = smap_get(&br->cfg->other_config, "hwaddr");
-    if (hwaddr && eth_addr_from_string(hwaddr, ea)) {
+    const char *hwaddr = smap_get_def(&br->cfg->other_config, "hwaddr", "");
+    if (eth_addr_from_string(hwaddr, ea)) {
         if (eth_addr_is_multicast(*ea)) {
             VLOG_ERR("bridge %s: cannot set MAC address to multicast "
                      "address "ETH_ADDR_FMT, br->name, ETH_ADDR_ARGS(*ea));
@@ -2100,8 +2038,8 @@  bridge_pick_datapath_id(struct bridge *br,
     const char *datapath_id;
     uint64_t dpid;
 
-    datapath_id = smap_get(&br->cfg->other_config, "datapath-id");
-    if (datapath_id && dpid_from_string(datapath_id, &dpid)) {
+    datapath_id = smap_get_def(&br->cfg->other_config, "datapath-id", "");
+    if (dpid_from_string(datapath_id, &dpid)) {
         return dpid;
     }
 
@@ -4152,8 +4090,8 @@  port_configure_lacp(struct port *port, struct lacp_settings *s)
                    ? priority
                    : UINT16_MAX - !ovs_list_is_short(&port->ifaces));
 
-    lacp_time = smap_get(&port->cfg->other_config, "lacp-time");
-    s->fast = lacp_time && !strcasecmp(lacp_time, "fast");
+    lacp_time = smap_get_def(&port->cfg->other_config, "lacp-time", "");
+    s->fast = !strcasecmp(lacp_time, "fast");
 
     s->fallback_ab_cfg = smap_get_bool(&port->cfg->other_config,
                                        "lacp-fallback-ab", false);