[ovs-dev] [PATCH 1/2] ovsdb: Save some space in the log for newly inserted records.

[Ben Pfaff]

On Fri, Jan 08, 2010 at 04:10:45PM -0800, Justin Pettit wrote:
> 
> On Jan 6, 2010, at 2:00 PM, Ben Pfaff wrote:
> 
> > When a new record is inserted into a database, ovsdb logs the values of all
> > of the fields in the record.  However, often new records have many columns
> > that contain default values.  There is no need to log those values, so this
> > commit causes them to be omitted.
> > 
> > As a side effect, this makes "ovsdb-tool show-log --more --more" output
> > easier to read, because record insertions print less noise.
> 
> The "--more" is actually implemented in the second patch.

Thanks, I updated the commit message to mention that.

> > +bool
> > +ovsdb_atom_is_default(const union ovsdb_atom *atom,
> > +                      enum ovsdb_atomic_type type)
> > +{
> > +    switch (type) {
> > +    case OVSDB_TYPE_VOID:
> > +        NOT_REACHED();
> 
> NOT_REACHED is just a #define to abort().  I'm a bit nervous about
> having this function potentially aborting a running process by someone
> not being careful about the data they pass into it.
> 
> > +    case OVSDB_N_TYPES:
> > +    default:
> > +        NOT_REACHED();
> 
> Same here.

The database has pretty exhaustive test cases.  How concerned are you?

I use this idiom elsewhere in that file and in the database code.  I
guess that, if we're concerned about it, then we should take care of it
all over the database code.  Do you think that is worthwhile?

> > +    for (i = 0; i < datum->n; i++) {
> > +        if (!ovsdb_atom_is_default(&datum->keys[i], type->key_type)) {
> > +            return false;
> > +        }
> > +        if (type->value_type != OVSDB_TYPE_VOID
> > +            && !ovsdb_atom_is_default(&datum->values[i], type->value_type)) {
> > +            return false;
> > +        }
> > +    }
> 
> As we discussed in person, it wasn't clear to me that a key always
> contains a value and that a value is only populated if the type is a
> mapping.  A bit of documentation in ovsdb-data.h would be really
> helpful.

That is a good idea.

I committed this:

commit c7239c2f29ac9b288a48bff7458f8709ee4f7b4b
Author: Ben Pfaff <blp at nicira.com>
Date:   Mon Jan 11 13:30:15 2010 -0800

    ovsdb: Improve comments.
    
    Suggested by Justin Pettit.

diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 18b8841..6ee5f9a 100644
*** a/lib/ovsdb-data.h
--- b/lib/ovsdb-data.h
***************
*** 73,79 ****
  struct json *ovsdb_atom_to_json(const union ovsdb_atom *,
                                  enum ovsdb_atomic_type);
  
! /* One value of an OVSDB type (given by struct ovsdb_type). */
  struct ovsdb_datum {
      unsigned int n;             /* Number of 'keys' and 'values'. */
      union ovsdb_atom *keys;     /* Each of the ovsdb_type's 'key_type'. */
--- 73,93 ----
  struct json *ovsdb_atom_to_json(const union ovsdb_atom *,
                                  enum ovsdb_atomic_type);
  
! /* An instance of an OVSDB type (given by struct ovsdb_type).
!  *
!  * 'n' is constrained by the ovsdb_type's 'n_min' and 'n_max'.
!  *
!  * If 'n' is nonzero, then 'keys' points to an array of 'n' atoms of the type
!  * specified by the ovsdb_type's 'key_type'.  (Otherwise, 'keys' should be
!  * null.)
!  *
!  * If 'n' is nonzero and the ovsdb_type's 'value_type' is not OVSDB_TYPE_VOID,
!  * then 'values' points to an array of 'n' atoms of the type specified by the
!  * 'value_type'.  (Otherwise, 'values' should be null.)
!  *
!  * Thus, for 'n' > 0, 'keys' will always be nonnull and 'values' will be
!  * nonnull only for "map" types.
!  */
  struct ovsdb_datum {
      unsigned int n;             /* Number of 'keys' and 'values'. */
      union ovsdb_atom *keys;     /* Each of the ovsdb_type's 'key_type'. */
diff --git a/lib/ovsdb-types.h b/lib/ovsdb-types.h
index 78d76c9..b9b3d5a 100644
*** a/lib/ovsdb-types.h
--- b/lib/ovsdb-types.h
***************
*** 1,4 ****
! /* Copyright (c) 2009 Nicira Networks
   *
   * Licensed under the Apache License, Version 2.0 (the "License");
   * you may not use this file except in compliance with the License.
--- 1,4 ----
! /* Copyright (c) 2009, 2010 Nicira Networks
   *
   * Licensed under the Apache License, Version 2.0 (the "License");
   * you may not use this file except in compliance with the License.
***************
*** 42,54 ****
  const char *ovsdb_atomic_type_to_string(enum ovsdb_atomic_type);
  struct json *ovsdb_atomic_type_to_json(enum ovsdb_atomic_type);
  
! /* An OVSDB type.  One of:
   *
!  *      - An atomic type.
   *
!  *      - A set of atomic types.
   *
!  *      - A map from one atomic type to another.
   */
  struct ovsdb_type {
      enum ovsdb_atomic_type key_type;
--- 42,63 ----
  const char *ovsdb_atomic_type_to_string(enum ovsdb_atomic_type);
  struct json *ovsdb_atomic_type_to_json(enum ovsdb_atomic_type);
  
! /* An OVSDB type.
   *
!  * Several rules constrain the valid types.  See ovsdb_type_is_valid() (in
!  * ovsdb-types.c) for details.
   *
!  * If 'value_type' is OVSDB_TYPE_VOID, 'n_min' is 1, and 'n_max' is 1, then the
!  * type is a single atomic 'key_type'.
   *
!  * If 'value_type' is OVSDB_TYPE_VOID and 'n_min' or 'n_max' (or both) has a
!  * value other than 1, then the type is a set of 'key_type'.  If 'n_min' is 0
!  * and 'n_max' is 1, then the type can also be considered an optional
!  * 'key_type'.
!  *
!  * If 'value_type' is not OVSDB_TYPE_VOID, then the type is a map from
!  * 'key_type' to 'value_type'.  If 'n_min' is 0 and 'n_max' is 1, then the type
!  * can also be considered an optional pair of 'key_type' and 'value_type'.
   */
  struct ovsdb_type {
      enum ovsdb_atomic_type key_type;