[ovs-dev,2/2] db-ctl-base: Give better error messages for ambiguous abbreviations.
diff mbox series

Message ID 20190918143252.3317-2-blp@ovn.org
State New
Headers show
Series
  • [ovs-dev,1/2] sset: New function sset_join().
Related show

Commit Message

Ben Pfaff Sept. 18, 2019, 2:32 p.m. UTC
Tables and columns may be abbreviated to unique prefixes, but until now
the error messages have just said there's more than one match.  This commit
makes the error messages list the possibilities.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/db-ctl-base.c  | 58 +++++++++++++++++++++++++++++-----------------
 tests/ovs-vsctl.at |  5 +++-
 2 files changed, 41 insertions(+), 22 deletions(-)

Comments

Ben Pfaff Sept. 18, 2019, 3:45 p.m. UTC | #1
THank you for the reviews.  I applied these to master.

On Wed, Sep 18, 2019 at 10:36:53AM -0700, Yifeng Sun wrote:
> LGTM, thanks.
> 
> Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
> 
> On Wed, Sep 18, 2019 at 9:32 AM Ben Pfaff <blp@ovn.org> wrote:
> >
> > Tables and columns may be abbreviated to unique prefixes, but until now
> > the error messages have just said there's more than one match.  This commit
> > makes the error messages list the possibilities.
> >
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  lib/db-ctl-base.c  | 58 +++++++++++++++++++++++++++++-----------------
> >  tests/ovs-vsctl.at |  5 +++-
> >  2 files changed, 41 insertions(+), 22 deletions(-)
> >
> > diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> > index 3bd9f006acb1..6ae638be5a2b 100644
> > --- a/lib/db-ctl-base.c
> > +++ b/lib/db-ctl-base.c
> > @@ -430,31 +430,39 @@ static char *
> >  get_column(const struct ovsdb_idl_table_class *table, const char *column_name,
> >             const struct ovsdb_idl_column **columnp)
> >  {
> > +    struct sset best_matches = SSET_INITIALIZER(&best_matches);
> >      const struct ovsdb_idl_column *best_match = NULL;
> >      unsigned int best_score = 0;
> > -    size_t i;
> >
> > -    for (i = 0; i < table->n_columns; i++) {
> > +    for (size_t i = 0; i < table->n_columns; i++) {
> >          const struct ovsdb_idl_column *column = &table->columns[i];
> >          unsigned int score = score_partial_match(column->name, column_name);
> > -        if (score > best_score) {
> > +        if (score && score >= best_score) {
> > +            if (score > best_score) {
> > +                sset_clear(&best_matches);
> > +            }
> > +            sset_add(&best_matches, column->name);
> >              best_match = column;
> >              best_score = score;
> > -        } else if (score == best_score) {
> > -            best_match = NULL;
> >          }
> >      }
> >
> > -    *columnp = best_match;
> > -    if (best_match) {
> > -        return NULL;
> > -    } else if (best_score) {
> > -        return xasprintf("%s contains more than one column whose name "
> > -                         "matches \"%s\"", table->name, column_name);
> > +    char *error = NULL;
> > +    *columnp = NULL;
> > +    if (!best_match) {
> > +        error = xasprintf("%s does not contain a column whose name matches "
> > +                          "\"%s\"", table->name, column_name);
> > +    } else if (sset_count(&best_matches) == 1) {
> > +        *columnp = best_match;
> >      } else {
> > -        return xasprintf("%s does not contain a column whose name matches "
> > -                         "\"%s\"", table->name, column_name);
> > +        char *matches = sset_join(&best_matches, ", ", "");
> > +        error = xasprintf("%s contains more than one column "
> > +                          "whose name matches \"%s\": %s",
> > +                          table->name, column_name, matches);
> > +        free(matches);
> >      }
> > +    sset_destroy(&best_matches);
> > +    return error;
> >  }
> >
> >  static char * OVS_WARN_UNUSED_RESULT
> > @@ -1207,27 +1215,35 @@ cmd_list(struct ctl_context *ctx)
> >  static char * OVS_WARN_UNUSED_RESULT
> >  get_table(const char *table_name, const struct ovsdb_idl_table_class **tablep)
> >  {
> > +    struct sset best_matches = SSET_INITIALIZER(&best_matches);
> >      const struct ovsdb_idl_table_class *best_match = NULL;
> >      unsigned int best_score = 0;
> > -    char *error = NULL;
> >
> >      for (const struct ovsdb_idl_table_class *table = idl_classes;
> >           table < &idl_classes[n_classes]; table++) {
> >          unsigned int score = score_partial_match(table->name, table_name);
> > -        if (score > best_score) {
> > +        if (score && score >= best_score) {
> > +            if (score > best_score) {
> > +                sset_clear(&best_matches);
> > +            }
> > +            sset_add(&best_matches, table->name);
> >              best_match = table;
> >              best_score = score;
> > -        } else if (score == best_score) {
> > -            best_match = NULL;
> >          }
> >      }
> > -    if (best_match) {
> > +
> > +    char *error = NULL;
> > +    if (!best_match) {
> > +        error = xasprintf("unknown table \"%s\"", table_name);
> > +    } else if (sset_count(&best_matches) == 1) {
> >          *tablep = best_match;
> > -    } else if (best_score) {
> > -        error = xasprintf("multiple table names match \"%s\"", table_name);
> >      } else {
> > -        error = xasprintf("unknown table \"%s\"", table_name);
> > +        char *matches = sset_join(&best_matches, ", ", "");
> > +        error = xasprintf("\"%s\" matches multiple table names: %s",
> > +                          table_name, matches);
> > +        free(matches);
> >      }
> > +    sset_destroy(&best_matches);
> >      return error;
> >  }
> >
> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index 4907be35d342..1b9c6d90219d 100644
> > --- a/tests/ovs-vsctl.at
> > +++ b/tests/ovs-vsctl.at
> > @@ -848,6 +848,9 @@ targets             : ["1.2.3.4:567"]
> >  AT_CHECK([RUN_OVS_VSCTL([list interx x])],
> >    [1], [], [ovs-vsctl: unknown table "interx"
> >  ])
> > +AT_CHECK([RUN_OVS_VSCTL([list c x])],
> > +  [1], [], [ovs-vsctl: "c" matches multiple table names: CT_Timeout_Policy, CT_Zone, Controller
> > +])
> >  AT_CHECK([RUN_OVS_VSCTL([list bridge x])],
> >    [1], [], [ovs-vsctl: no row "x" in table Bridge
> >  ])
> > @@ -855,7 +858,7 @@ AT_CHECK([RUN_OVS_VSCTL([get bridge x datapath_id])],
> >    [1], [], [ovs-vsctl: no row "x" in table Bridge
> >  ])
> >  AT_CHECK([RUN_OVS_VSCTL([get bridge br0 d])],
> > -  [1], [], [ovs-vsctl: Bridge contains more than one column whose name matches "d"
> > +  [1], [], [ovs-vsctl: Bridge contains more than one column whose name matches "d": datapath_id, datapath_type, datapath_version
> >  ])
> >  AT_CHECK([RUN_OVS_VSCTL([get bridge br0 x])],
> >    [1], [], [ovs-vsctl: Bridge does not contain a column whose name matches "x"
> > --
> > 2.21.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Yifeng Sun Sept. 18, 2019, 5:36 p.m. UTC | #2
LGTM, thanks.

Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>

On Wed, Sep 18, 2019 at 9:32 AM Ben Pfaff <blp@ovn.org> wrote:
>
> Tables and columns may be abbreviated to unique prefixes, but until now
> the error messages have just said there's more than one match.  This commit
> makes the error messages list the possibilities.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  lib/db-ctl-base.c  | 58 +++++++++++++++++++++++++++++-----------------
>  tests/ovs-vsctl.at |  5 +++-
>  2 files changed, 41 insertions(+), 22 deletions(-)
>
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index 3bd9f006acb1..6ae638be5a2b 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -430,31 +430,39 @@ static char *
>  get_column(const struct ovsdb_idl_table_class *table, const char *column_name,
>             const struct ovsdb_idl_column **columnp)
>  {
> +    struct sset best_matches = SSET_INITIALIZER(&best_matches);
>      const struct ovsdb_idl_column *best_match = NULL;
>      unsigned int best_score = 0;
> -    size_t i;
>
> -    for (i = 0; i < table->n_columns; i++) {
> +    for (size_t i = 0; i < table->n_columns; i++) {
>          const struct ovsdb_idl_column *column = &table->columns[i];
>          unsigned int score = score_partial_match(column->name, column_name);
> -        if (score > best_score) {
> +        if (score && score >= best_score) {
> +            if (score > best_score) {
> +                sset_clear(&best_matches);
> +            }
> +            sset_add(&best_matches, column->name);
>              best_match = column;
>              best_score = score;
> -        } else if (score == best_score) {
> -            best_match = NULL;
>          }
>      }
>
> -    *columnp = best_match;
> -    if (best_match) {
> -        return NULL;
> -    } else if (best_score) {
> -        return xasprintf("%s contains more than one column whose name "
> -                         "matches \"%s\"", table->name, column_name);
> +    char *error = NULL;
> +    *columnp = NULL;
> +    if (!best_match) {
> +        error = xasprintf("%s does not contain a column whose name matches "
> +                          "\"%s\"", table->name, column_name);
> +    } else if (sset_count(&best_matches) == 1) {
> +        *columnp = best_match;
>      } else {
> -        return xasprintf("%s does not contain a column whose name matches "
> -                         "\"%s\"", table->name, column_name);
> +        char *matches = sset_join(&best_matches, ", ", "");
> +        error = xasprintf("%s contains more than one column "
> +                          "whose name matches \"%s\": %s",
> +                          table->name, column_name, matches);
> +        free(matches);
>      }
> +    sset_destroy(&best_matches);
> +    return error;
>  }
>
>  static char * OVS_WARN_UNUSED_RESULT
> @@ -1207,27 +1215,35 @@ cmd_list(struct ctl_context *ctx)
>  static char * OVS_WARN_UNUSED_RESULT
>  get_table(const char *table_name, const struct ovsdb_idl_table_class **tablep)
>  {
> +    struct sset best_matches = SSET_INITIALIZER(&best_matches);
>      const struct ovsdb_idl_table_class *best_match = NULL;
>      unsigned int best_score = 0;
> -    char *error = NULL;
>
>      for (const struct ovsdb_idl_table_class *table = idl_classes;
>           table < &idl_classes[n_classes]; table++) {
>          unsigned int score = score_partial_match(table->name, table_name);
> -        if (score > best_score) {
> +        if (score && score >= best_score) {
> +            if (score > best_score) {
> +                sset_clear(&best_matches);
> +            }
> +            sset_add(&best_matches, table->name);
>              best_match = table;
>              best_score = score;
> -        } else if (score == best_score) {
> -            best_match = NULL;
>          }
>      }
> -    if (best_match) {
> +
> +    char *error = NULL;
> +    if (!best_match) {
> +        error = xasprintf("unknown table \"%s\"", table_name);
> +    } else if (sset_count(&best_matches) == 1) {
>          *tablep = best_match;
> -    } else if (best_score) {
> -        error = xasprintf("multiple table names match \"%s\"", table_name);
>      } else {
> -        error = xasprintf("unknown table \"%s\"", table_name);
> +        char *matches = sset_join(&best_matches, ", ", "");
> +        error = xasprintf("\"%s\" matches multiple table names: %s",
> +                          table_name, matches);
> +        free(matches);
>      }
> +    sset_destroy(&best_matches);
>      return error;
>  }
>
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index 4907be35d342..1b9c6d90219d 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -848,6 +848,9 @@ targets             : ["1.2.3.4:567"]
>  AT_CHECK([RUN_OVS_VSCTL([list interx x])],
>    [1], [], [ovs-vsctl: unknown table "interx"
>  ])
> +AT_CHECK([RUN_OVS_VSCTL([list c x])],
> +  [1], [], [ovs-vsctl: "c" matches multiple table names: CT_Timeout_Policy, CT_Zone, Controller
> +])
>  AT_CHECK([RUN_OVS_VSCTL([list bridge x])],
>    [1], [], [ovs-vsctl: no row "x" in table Bridge
>  ])
> @@ -855,7 +858,7 @@ AT_CHECK([RUN_OVS_VSCTL([get bridge x datapath_id])],
>    [1], [], [ovs-vsctl: no row "x" in table Bridge
>  ])
>  AT_CHECK([RUN_OVS_VSCTL([get bridge br0 d])],
> -  [1], [], [ovs-vsctl: Bridge contains more than one column whose name matches "d"
> +  [1], [], [ovs-vsctl: Bridge contains more than one column whose name matches "d": datapath_id, datapath_type, datapath_version
>  ])
>  AT_CHECK([RUN_OVS_VSCTL([get bridge br0 x])],
>    [1], [], [ovs-vsctl: Bridge does not contain a column whose name matches "x"
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox series

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index 3bd9f006acb1..6ae638be5a2b 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -430,31 +430,39 @@  static char *
 get_column(const struct ovsdb_idl_table_class *table, const char *column_name,
            const struct ovsdb_idl_column **columnp)
 {
+    struct sset best_matches = SSET_INITIALIZER(&best_matches);
     const struct ovsdb_idl_column *best_match = NULL;
     unsigned int best_score = 0;
-    size_t i;
 
-    for (i = 0; i < table->n_columns; i++) {
+    for (size_t i = 0; i < table->n_columns; i++) {
         const struct ovsdb_idl_column *column = &table->columns[i];
         unsigned int score = score_partial_match(column->name, column_name);
-        if (score > best_score) {
+        if (score && score >= best_score) {
+            if (score > best_score) {
+                sset_clear(&best_matches);
+            }
+            sset_add(&best_matches, column->name);
             best_match = column;
             best_score = score;
-        } else if (score == best_score) {
-            best_match = NULL;
         }
     }
 
-    *columnp = best_match;
-    if (best_match) {
-        return NULL;
-    } else if (best_score) {
-        return xasprintf("%s contains more than one column whose name "
-                         "matches \"%s\"", table->name, column_name);
+    char *error = NULL;
+    *columnp = NULL;
+    if (!best_match) {
+        error = xasprintf("%s does not contain a column whose name matches "
+                          "\"%s\"", table->name, column_name);
+    } else if (sset_count(&best_matches) == 1) {
+        *columnp = best_match;
     } else {
-        return xasprintf("%s does not contain a column whose name matches "
-                         "\"%s\"", table->name, column_name);
+        char *matches = sset_join(&best_matches, ", ", "");
+        error = xasprintf("%s contains more than one column "
+                          "whose name matches \"%s\": %s",
+                          table->name, column_name, matches);
+        free(matches);
     }
+    sset_destroy(&best_matches);
+    return error;
 }
 
 static char * OVS_WARN_UNUSED_RESULT
@@ -1207,27 +1215,35 @@  cmd_list(struct ctl_context *ctx)
 static char * OVS_WARN_UNUSED_RESULT
 get_table(const char *table_name, const struct ovsdb_idl_table_class **tablep)
 {
+    struct sset best_matches = SSET_INITIALIZER(&best_matches);
     const struct ovsdb_idl_table_class *best_match = NULL;
     unsigned int best_score = 0;
-    char *error = NULL;
 
     for (const struct ovsdb_idl_table_class *table = idl_classes;
          table < &idl_classes[n_classes]; table++) {
         unsigned int score = score_partial_match(table->name, table_name);
-        if (score > best_score) {
+        if (score && score >= best_score) {
+            if (score > best_score) {
+                sset_clear(&best_matches);
+            }
+            sset_add(&best_matches, table->name);
             best_match = table;
             best_score = score;
-        } else if (score == best_score) {
-            best_match = NULL;
         }
     }
-    if (best_match) {
+
+    char *error = NULL;
+    if (!best_match) {
+        error = xasprintf("unknown table \"%s\"", table_name);
+    } else if (sset_count(&best_matches) == 1) {
         *tablep = best_match;
-    } else if (best_score) {
-        error = xasprintf("multiple table names match \"%s\"", table_name);
     } else {
-        error = xasprintf("unknown table \"%s\"", table_name);
+        char *matches = sset_join(&best_matches, ", ", "");
+        error = xasprintf("\"%s\" matches multiple table names: %s",
+                          table_name, matches);
+        free(matches);
     }
+    sset_destroy(&best_matches);
     return error;
 }
 
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index 4907be35d342..1b9c6d90219d 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -848,6 +848,9 @@  targets             : ["1.2.3.4:567"]
 AT_CHECK([RUN_OVS_VSCTL([list interx x])],
   [1], [], [ovs-vsctl: unknown table "interx"
 ])
+AT_CHECK([RUN_OVS_VSCTL([list c x])],
+  [1], [], [ovs-vsctl: "c" matches multiple table names: CT_Timeout_Policy, CT_Zone, Controller
+])
 AT_CHECK([RUN_OVS_VSCTL([list bridge x])],
   [1], [], [ovs-vsctl: no row "x" in table Bridge
 ])
@@ -855,7 +858,7 @@  AT_CHECK([RUN_OVS_VSCTL([get bridge x datapath_id])],
   [1], [], [ovs-vsctl: no row "x" in table Bridge
 ])
 AT_CHECK([RUN_OVS_VSCTL([get bridge br0 d])],
-  [1], [], [ovs-vsctl: Bridge contains more than one column whose name matches "d"
+  [1], [], [ovs-vsctl: Bridge contains more than one column whose name matches "d": datapath_id, datapath_type, datapath_version
 ])
 AT_CHECK([RUN_OVS_VSCTL([get bridge br0 x])],
   [1], [], [ovs-vsctl: Bridge does not contain a column whose name matches "x"