diff mbox series

[ovs-dev,v2,2/9] ovsdb: storage: Allow setting the name for the unbacked storage.

Message ID 20210612020008.3944088-3-i.maximets@ovn.org
State Superseded
Headers show
Series OVSDB Relay Service Model. (Was: OVSDB 2-Tier deployment) | expand

Commit Message

Ilya Maximets June 12, 2021, 2 a.m. UTC
ovsdb_create() requires schema or storage to be nonnull, but in
practice it requires to have schema name or a storage name to
use it as a database name.  Only clustered storage has a name.
This means that only clustered database can be created without
schema,  Changing that by allowing unbacked storage to have a
name.  This way we can create database with unbacked storage
without schema.  Will be used in next commits to create database
for ovsdb 'relay' service model.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 ovsdb/file.c         |  2 +-
 ovsdb/ovsdb-server.c |  2 +-
 ovsdb/storage.c      | 13 ++++++++++---
 ovsdb/storage.h      |  2 +-
 tests/test-ovsdb.c   |  6 +++---
 5 files changed, 16 insertions(+), 9 deletions(-)

Comments

Mark Gray June 19, 2021, 5:28 p.m. UTC | #1
On 12/06/2021 03:00, Ilya Maximets wrote:
> ovsdb_create() requires schema or storage to be nonnull, but in
> practice it requires to have schema name or a storage name to
> use it as a database name.  Only clustered storage has a name.
> This means that only clustered database can be created without
> schema,  Changing that by allowing unbacked storage to have a
> name.  This way we can create database with unbacked storage
> without schema.  Will be used in next commits to create database
> for ovsdb 'relay' service model.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  ovsdb/file.c         |  2 +-
>  ovsdb/ovsdb-server.c |  2 +-
>  ovsdb/storage.c      | 13 ++++++++++---
>  ovsdb/storage.h      |  2 +-
>  tests/test-ovsdb.c   |  6 +++---
>  5 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/ovsdb/file.c b/ovsdb/file.c
> index 0b8bdfe37..59220824f 100644
> --- a/ovsdb/file.c
> +++ b/ovsdb/file.c
> @@ -318,7 +318,7 @@ ovsdb_convert(const struct ovsdb *src, const struct ovsdb_schema *new_schema,
>                struct ovsdb **dstp)
>  {
>      struct ovsdb *dst = ovsdb_create(ovsdb_schema_clone(new_schema),
> -                                     ovsdb_storage_create_unbacked());
> +                                     ovsdb_storage_create_unbacked(NULL));
>      struct ovsdb_txn *txn = ovsdb_txn_create(dst);
>      struct ovsdb_error *error = NULL;
>  
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index b09232c65..23bd226a3 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -738,7 +738,7 @@ add_server_db(struct server_config *config)
>      /* We don't need txn_history for server_db. */
>  
>      db->filename = xstrdup("<internal>");
> -    db->db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
> +    db->db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
>      bool ok OVS_UNUSED = ovsdb_jsonrpc_server_add_db(config->jsonrpc, db->db);
>      ovs_assert(ok);
>      add_db(config, db);
> diff --git a/ovsdb/storage.c b/ovsdb/storage.c
> index 40415fcf6..d727b1eac 100644
> --- a/ovsdb/storage.c
> +++ b/ovsdb/storage.c
> @@ -45,6 +45,8 @@ struct ovsdb_storage {
>      struct ovsdb_log *log;
>      struct raft *raft;
>  
> +    char *unbacked_name; /* Name of the unbacked storage. */
> +

Bit of a nit but should this be a "const char *" type?

>      /* All kinds of storage. */
>      struct ovsdb_error *error;  /* If nonnull, a permanent error. */
>      long long next_snapshot_min; /* Earliest time to take next snapshot. */
> @@ -121,12 +123,14 @@ ovsdb_storage_open_standalone(const char *filename, bool rw)
>  }
>  
>  /* Creates and returns new storage without any backing.  Nothing will be read
> - * from the storage, and writes are discarded. */
> + * from the storage, and writes are discarded.  If 'name' is nonnull, it will
> + * be used as a storage name. */
>  struct ovsdb_storage *
> -ovsdb_storage_create_unbacked(void)
> +ovsdb_storage_create_unbacked(const char *name)
>  {
>      struct ovsdb_storage *storage = xzalloc(sizeof *storage);
>      schedule_next_snapshot(storage, false);
> +    storage->unbacked_name = nullable_xstrdup(name);
>      return storage;
>  }
>  
> @@ -137,6 +141,7 @@ ovsdb_storage_close(struct ovsdb_storage *storage)
>          ovsdb_log_close(storage->log);
>          raft_close(storage->raft);
>          ovsdb_error_destroy(storage->error);
> +        free(storage->unbacked_name);

Can this ^ not be NULL?

>          free(storage);
>      }
>  }
> @@ -230,7 +235,9 @@ ovsdb_storage_wait(struct ovsdb_storage *storage)
>  const char *
>  ovsdb_storage_get_name(const struct ovsdb_storage *storage)
>  {
> -    return storage->raft ? raft_get_name(storage->raft) : NULL;
> +    return storage->unbacked_name ? storage->unbacked_name
> +           : storage->raft ? raft_get_name(storage->raft)
> +           : NULL;
>  }
>  
>  /* Attempts to read a log record from 'storage'.
> diff --git a/ovsdb/storage.h b/ovsdb/storage.h
> index 02b6e7e6c..e120094d7 100644
> --- a/ovsdb/storage.h
> +++ b/ovsdb/storage.h
> @@ -29,7 +29,7 @@ struct uuid;
>  struct ovsdb_error *ovsdb_storage_open(const char *filename, bool rw,
>                                         struct ovsdb_storage **)
>      OVS_WARN_UNUSED_RESULT;
> -struct ovsdb_storage *ovsdb_storage_create_unbacked(void);
> +struct ovsdb_storage *ovsdb_storage_create_unbacked(const char *name);
>  void ovsdb_storage_close(struct ovsdb_storage *);
>  
>  const char *ovsdb_storage_get_model(const struct ovsdb_storage *);
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index a886f971e..fb6b3acca 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -1485,7 +1485,7 @@ do_execute__(struct ovs_cmdl_context *ctx, bool ro)
>      json = parse_json(ctx->argv[1]);
>      check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
>      json_destroy(json);
> -    db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
> +    db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
>  
>      for (i = 2; i < ctx->argc; i++) {
>          struct json *params, *result;
> @@ -1551,7 +1551,7 @@ do_trigger(struct ovs_cmdl_context *ctx)
>      json = parse_json(ctx->argv[1]);
>      check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
>      json_destroy(json);
> -    db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
> +    db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));

I think destroying the ovsdb_storage object will trigger a segfault.
Maybe you should add a call to ovsdb_storage_close(struct ovsdb_storage
*storage) in order to test that path.
>  
>      ovsdb_server_init(&server);
>      ovsdb_server_add_db(&server, db);
> @@ -1781,7 +1781,7 @@ do_transact(struct ovs_cmdl_context *ctx)
>                        "       \"j\": {\"type\": \"integer\"}}}}}");
>      check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
>      json_destroy(json);
> -    do_transact_db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
> +    do_transact_db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
>      do_transact_table = ovsdb_get_table(do_transact_db, "mytable");
>      ovs_assert(do_transact_table != NULL);
>  
>
Ilya Maximets June 19, 2021, 6:29 p.m. UTC | #2
On 6/19/21 7:28 PM, Mark Gray wrote:
> On 12/06/2021 03:00, Ilya Maximets wrote:
>> ovsdb_create() requires schema or storage to be nonnull, but in
>> practice it requires to have schema name or a storage name to
>> use it as a database name.  Only clustered storage has a name.
>> This means that only clustered database can be created without
>> schema,  Changing that by allowing unbacked storage to have a
>> name.  This way we can create database with unbacked storage
>> without schema.  Will be used in next commits to create database
>> for ovsdb 'relay' service model.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>> ---
>>  ovsdb/file.c         |  2 +-
>>  ovsdb/ovsdb-server.c |  2 +-
>>  ovsdb/storage.c      | 13 ++++++++++---
>>  ovsdb/storage.h      |  2 +-
>>  tests/test-ovsdb.c   |  6 +++---
>>  5 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/ovsdb/file.c b/ovsdb/file.c
>> index 0b8bdfe37..59220824f 100644
>> --- a/ovsdb/file.c
>> +++ b/ovsdb/file.c
>> @@ -318,7 +318,7 @@ ovsdb_convert(const struct ovsdb *src, const struct ovsdb_schema *new_schema,
>>                struct ovsdb **dstp)
>>  {
>>      struct ovsdb *dst = ovsdb_create(ovsdb_schema_clone(new_schema),
>> -                                     ovsdb_storage_create_unbacked());
>> +                                     ovsdb_storage_create_unbacked(NULL));
>>      struct ovsdb_txn *txn = ovsdb_txn_create(dst);
>>      struct ovsdb_error *error = NULL;
>>  
>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>> index b09232c65..23bd226a3 100644
>> --- a/ovsdb/ovsdb-server.c
>> +++ b/ovsdb/ovsdb-server.c
>> @@ -738,7 +738,7 @@ add_server_db(struct server_config *config)
>>      /* We don't need txn_history for server_db. */
>>  
>>      db->filename = xstrdup("<internal>");
>> -    db->db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
>> +    db->db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
>>      bool ok OVS_UNUSED = ovsdb_jsonrpc_server_add_db(config->jsonrpc, db->db);
>>      ovs_assert(ok);
>>      add_db(config, db);
>> diff --git a/ovsdb/storage.c b/ovsdb/storage.c
>> index 40415fcf6..d727b1eac 100644
>> --- a/ovsdb/storage.c
>> +++ b/ovsdb/storage.c
>> @@ -45,6 +45,8 @@ struct ovsdb_storage {
>>      struct ovsdb_log *log;
>>      struct raft *raft;
>>  
>> +    char *unbacked_name; /* Name of the unbacked storage. */
>> +
> 
> Bit of a nit but should this be a "const char *" type?

I can do that, but that measn adding a CONST_CASTs in other places,
otherwise compiler would complain about discarded qualifier, like
in case of free():

ovsdb/storage.c:144:14: error: passing 'const char *' to parameter
                        of type 'void *' discards qualifiers
        free(storage->unbacked_name);
             ^~~~~~~~~~~~~~~~~~~~~~

So, I'm not sure it it worth complications.

> 
>>      /* All kinds of storage. */
>>      struct ovsdb_error *error;  /* If nonnull, a permanent error. */
>>      long long next_snapshot_min; /* Earliest time to take next snapshot. */
>> @@ -121,12 +123,14 @@ ovsdb_storage_open_standalone(const char *filename, bool rw)
>>  }
>>  
>>  /* Creates and returns new storage without any backing.  Nothing will be read
>> - * from the storage, and writes are discarded. */
>> + * from the storage, and writes are discarded.  If 'name' is nonnull, it will
>> + * be used as a storage name. */
>>  struct ovsdb_storage *
>> -ovsdb_storage_create_unbacked(void)
>> +ovsdb_storage_create_unbacked(const char *name)
>>  {
>>      struct ovsdb_storage *storage = xzalloc(sizeof *storage);
>>      schedule_next_snapshot(storage, false);
>> +    storage->unbacked_name = nullable_xstrdup(name);
>>      return storage;
>>  }
>>  
>> @@ -137,6 +141,7 @@ ovsdb_storage_close(struct ovsdb_storage *storage)
>>          ovsdb_log_close(storage->log);
>>          raft_close(storage->raft);
>>          ovsdb_error_destroy(storage->error);
>> +        free(storage->unbacked_name);
> 
> Can this ^ not be NULL?

This is a NULL in most cases, because allocated with xzalloc and
non-NULL for relays.  But there is no problem with that.
free(NULL) is a no-op according to the C standard and not checking
for NULL is encouraged by the coding style.

> 
>>          free(storage);
>>      }
>>  }
>> @@ -230,7 +235,9 @@ ovsdb_storage_wait(struct ovsdb_storage *storage)
>>  const char *
>>  ovsdb_storage_get_name(const struct ovsdb_storage *storage)
>>  {
>> -    return storage->raft ? raft_get_name(storage->raft) : NULL;
>> +    return storage->unbacked_name ? storage->unbacked_name
>> +           : storage->raft ? raft_get_name(storage->raft)
>> +           : NULL;
>>  }
>>  
>>  /* Attempts to read a log record from 'storage'.
>> diff --git a/ovsdb/storage.h b/ovsdb/storage.h
>> index 02b6e7e6c..e120094d7 100644
>> --- a/ovsdb/storage.h
>> +++ b/ovsdb/storage.h
>> @@ -29,7 +29,7 @@ struct uuid;
>>  struct ovsdb_error *ovsdb_storage_open(const char *filename, bool rw,
>>                                         struct ovsdb_storage **)
>>      OVS_WARN_UNUSED_RESULT;
>> -struct ovsdb_storage *ovsdb_storage_create_unbacked(void);
>> +struct ovsdb_storage *ovsdb_storage_create_unbacked(const char *name);
>>  void ovsdb_storage_close(struct ovsdb_storage *);
>>  
>>  const char *ovsdb_storage_get_model(const struct ovsdb_storage *);
>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
>> index a886f971e..fb6b3acca 100644
>> --- a/tests/test-ovsdb.c
>> +++ b/tests/test-ovsdb.c
>> @@ -1485,7 +1485,7 @@ do_execute__(struct ovs_cmdl_context *ctx, bool ro)
>>      json = parse_json(ctx->argv[1]);
>>      check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
>>      json_destroy(json);
>> -    db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
>> +    db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
>>  
>>      for (i = 2; i < ctx->argc; i++) {
>>          struct json *params, *result;
>> @@ -1551,7 +1551,7 @@ do_trigger(struct ovs_cmdl_context *ctx)
>>      json = parse_json(ctx->argv[1]);
>>      check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
>>      json_destroy(json);
>> -    db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
>> +    db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
> 
> I think destroying the ovsdb_storage object will trigger a segfault.
> Maybe you should add a call to ovsdb_storage_close(struct ovsdb_storage
> *storage) in order to test that path.

ovsdb_storage_close() is called as part of ovsdb_destroy, so it's
tested.

>>  
>>      ovsdb_server_init(&server);
>>      ovsdb_server_add_db(&server, db);
>> @@ -1781,7 +1781,7 @@ do_transact(struct ovs_cmdl_context *ctx)
>>                        "       \"j\": {\"type\": \"integer\"}}}}}");
>>      check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
>>      json_destroy(json);
>> -    do_transact_db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
>> +    do_transact_db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
>>      do_transact_table = ovsdb_get_table(do_transact_db, "mytable");
>>      ovs_assert(do_transact_table != NULL);
>>  
>>
>
Dumitru Ceara June 25, 2021, 1:34 p.m. UTC | #3
On 6/12/21 4:00 AM, Ilya Maximets wrote:
> ovsdb_create() requires schema or storage to be nonnull, but in
> practice it requires to have schema name or a storage name to
> use it as a database name.  Only clustered storage has a name.
> This means that only clustered database can be created without
> schema,  Changing that by allowing unbacked storage to have a
> name.  This way we can create database with unbacked storage
> without schema.  Will be used in next commits to create database
> for ovsdb 'relay' service model.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks!
diff mbox series

Patch

diff --git a/ovsdb/file.c b/ovsdb/file.c
index 0b8bdfe37..59220824f 100644
--- a/ovsdb/file.c
+++ b/ovsdb/file.c
@@ -318,7 +318,7 @@  ovsdb_convert(const struct ovsdb *src, const struct ovsdb_schema *new_schema,
               struct ovsdb **dstp)
 {
     struct ovsdb *dst = ovsdb_create(ovsdb_schema_clone(new_schema),
-                                     ovsdb_storage_create_unbacked());
+                                     ovsdb_storage_create_unbacked(NULL));
     struct ovsdb_txn *txn = ovsdb_txn_create(dst);
     struct ovsdb_error *error = NULL;
 
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index b09232c65..23bd226a3 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -738,7 +738,7 @@  add_server_db(struct server_config *config)
     /* We don't need txn_history for server_db. */
 
     db->filename = xstrdup("<internal>");
-    db->db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
+    db->db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
     bool ok OVS_UNUSED = ovsdb_jsonrpc_server_add_db(config->jsonrpc, db->db);
     ovs_assert(ok);
     add_db(config, db);
diff --git a/ovsdb/storage.c b/ovsdb/storage.c
index 40415fcf6..d727b1eac 100644
--- a/ovsdb/storage.c
+++ b/ovsdb/storage.c
@@ -45,6 +45,8 @@  struct ovsdb_storage {
     struct ovsdb_log *log;
     struct raft *raft;
 
+    char *unbacked_name; /* Name of the unbacked storage. */
+
     /* All kinds of storage. */
     struct ovsdb_error *error;  /* If nonnull, a permanent error. */
     long long next_snapshot_min; /* Earliest time to take next snapshot. */
@@ -121,12 +123,14 @@  ovsdb_storage_open_standalone(const char *filename, bool rw)
 }
 
 /* Creates and returns new storage without any backing.  Nothing will be read
- * from the storage, and writes are discarded. */
+ * from the storage, and writes are discarded.  If 'name' is nonnull, it will
+ * be used as a storage name. */
 struct ovsdb_storage *
-ovsdb_storage_create_unbacked(void)
+ovsdb_storage_create_unbacked(const char *name)
 {
     struct ovsdb_storage *storage = xzalloc(sizeof *storage);
     schedule_next_snapshot(storage, false);
+    storage->unbacked_name = nullable_xstrdup(name);
     return storage;
 }
 
@@ -137,6 +141,7 @@  ovsdb_storage_close(struct ovsdb_storage *storage)
         ovsdb_log_close(storage->log);
         raft_close(storage->raft);
         ovsdb_error_destroy(storage->error);
+        free(storage->unbacked_name);
         free(storage);
     }
 }
@@ -230,7 +235,9 @@  ovsdb_storage_wait(struct ovsdb_storage *storage)
 const char *
 ovsdb_storage_get_name(const struct ovsdb_storage *storage)
 {
-    return storage->raft ? raft_get_name(storage->raft) : NULL;
+    return storage->unbacked_name ? storage->unbacked_name
+           : storage->raft ? raft_get_name(storage->raft)
+           : NULL;
 }
 
 /* Attempts to read a log record from 'storage'.
diff --git a/ovsdb/storage.h b/ovsdb/storage.h
index 02b6e7e6c..e120094d7 100644
--- a/ovsdb/storage.h
+++ b/ovsdb/storage.h
@@ -29,7 +29,7 @@  struct uuid;
 struct ovsdb_error *ovsdb_storage_open(const char *filename, bool rw,
                                        struct ovsdb_storage **)
     OVS_WARN_UNUSED_RESULT;
-struct ovsdb_storage *ovsdb_storage_create_unbacked(void);
+struct ovsdb_storage *ovsdb_storage_create_unbacked(const char *name);
 void ovsdb_storage_close(struct ovsdb_storage *);
 
 const char *ovsdb_storage_get_model(const struct ovsdb_storage *);
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index a886f971e..fb6b3acca 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -1485,7 +1485,7 @@  do_execute__(struct ovs_cmdl_context *ctx, bool ro)
     json = parse_json(ctx->argv[1]);
     check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
     json_destroy(json);
-    db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
+    db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
 
     for (i = 2; i < ctx->argc; i++) {
         struct json *params, *result;
@@ -1551,7 +1551,7 @@  do_trigger(struct ovs_cmdl_context *ctx)
     json = parse_json(ctx->argv[1]);
     check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
     json_destroy(json);
-    db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
+    db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
 
     ovsdb_server_init(&server);
     ovsdb_server_add_db(&server, db);
@@ -1781,7 +1781,7 @@  do_transact(struct ovs_cmdl_context *ctx)
                       "       \"j\": {\"type\": \"integer\"}}}}}");
     check_ovsdb_error(ovsdb_schema_from_json(json, &schema));
     json_destroy(json);
-    do_transact_db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
+    do_transact_db = ovsdb_create(schema, ovsdb_storage_create_unbacked(NULL));
     do_transact_table = ovsdb_get_table(do_transact_db, "mytable");
     ovs_assert(do_transact_table != NULL);