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