| Message ID | 1456992808-27582-2-git-send-email-azhou@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Mar 03, 2016 at 12:13:21AM -0800, Andy Zhou wrote: > It turns out there is no need for ovsdb_jsonrcp_session to have access s/jsonrcp/jsonrpc/ > to the remote data structure. Make it opaque as a 'void *' pointer. > The pointer value is still useful when selecting ovsdb_jsonrpc_sessions > that of the same 'remote'. This change will laster multi-threading s/laster/later/ > patches easier because access to 'remote' does not need be protected. > > Signed-off-by: Andy Zhou <azhou@ovn.org> I am not sure I understand the benefit yet. The definition of a struct does not have to be visible for a piece of code to refer to it through a pointer. Also, using a void pointer loses a lot of type safety. Can you explain further? Thanks, Ben.
On Fri, Mar 18, 2016 at 5:20 PM, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Mar 03, 2016 at 12:13:21AM -0800, Andy Zhou wrote: > > It turns out there is no need for ovsdb_jsonrcp_session to have access > > s/jsonrcp/jsonrpc/ > > > to the remote data structure. Make it opaque as a 'void *' pointer. > > The pointer value is still useful when selecting ovsdb_jsonrpc_sessions > > that of the same 'remote'. This change will laster multi-threading > > s/laster/later/ > > > patches easier because access to 'remote' does not need be protected. > > > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > I am not sure I understand the benefit yet. The definition of a struct > does not have to be visible for a piece of code to refer to it through a > pointer. Also, using a void pointer loses a lot of type safety. > > Can you explain further? > Yes, I agree this is a trade off. Wonder if there is better way. Currently 'remote' object are only handled within the main thread, so no locking is required to access those objects. The intention of changing to 'void' is to avoid 'sessions thread' code accidentally dereference 'remote' object, thus cause race conditions. I was thinking the trade off is worthwhile... Please let me know if you don't agree. > >
On Mon, Mar 21, 2016 at 01:00:53PM -0700, Andy Zhou wrote: > On Fri, Mar 18, 2016 at 5:20 PM, Ben Pfaff <blp@ovn.org> wrote: > > > On Thu, Mar 03, 2016 at 12:13:21AM -0800, Andy Zhou wrote: > > > It turns out there is no need for ovsdb_jsonrcp_session to have access > > > > s/jsonrcp/jsonrpc/ > > > > > to the remote data structure. Make it opaque as a 'void *' pointer. > > > The pointer value is still useful when selecting ovsdb_jsonrpc_sessions > > > that of the same 'remote'. This change will laster multi-threading > > > > s/laster/later/ > > > > > patches easier because access to 'remote' does not need be protected. > > > > > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > > > I am not sure I understand the benefit yet. The definition of a struct > > does not have to be visible for a piece of code to refer to it through a > > pointer. Also, using a void pointer loses a lot of type safety. > > > > Can you explain further? > > > > Yes, I agree this is a trade off. Wonder if there is better way. > > Currently 'remote' object are only handled within the main thread, so no > locking > is required to access those objects. The intention of changing to 'void' > is to avoid > 'sessions thread' code accidentally dereference 'remote' object, thus cause > race > conditions. I was thinking the trade off is worthwhile... Please let me > know if you > don't agree. I think that there is probably a better way than a void pointer. Usually, code can be arranged so that code that should not have a visible definition does not; for example, by putting the definition after the code that should not see the definition. Is that difficult here?
diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c index 56dddc6..660250d 100644 --- a/ovsdb/jsonrpc-server.c +++ b/ovsdb/jsonrpc-server.c @@ -56,14 +56,14 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); static void ovsdb_jsonrpc_sessions_run(struct ovs_list *); static void ovsdb_jsonrpc_sessions_wait(struct ovs_list *); static void ovsdb_jsonrpc_sessions_close(struct ovs_list *, - const struct ovsdb_jsonrpc_remote *remote); -static void ovsdb_jsonrpc_sessions_reconnect( struct ovs_list *, - const struct ovsdb_jsonrpc_remote *remote); + const void *remote); +static void ovsdb_jsonrpc_sessions_reconnect(struct ovs_list *, + const void *remote); static void ovsdb_jsonrpc_sessions_set_options(struct ovs_list *, - const struct ovsdb_jsonrpc_remote *remote, + const void *remote, const struct ovsdb_jsonrpc_options *); static size_t ovsdb_jsonrpc_sessions_count(const struct ovs_list *, - const struct ovsdb_jsonrpc_remote *remote); + const void *remote); /* Sessions. */ static struct ovsdb_jsonrpc_session *ovsdb_jsonrpc_session_create( @@ -408,7 +408,7 @@ ovsdb_jsonrpc_server_get_memory_usage(const struct ovsdb_jsonrpc_server *svr, struct ovsdb_jsonrpc_session { struct ovs_list node; /* Element in remote's sessions list. */ struct ovsdb_session up; - struct ovsdb_jsonrpc_remote *remote; + const void *remote; /* Triggers. */ struct hmap triggers; /* Hmap of "struct ovsdb_jsonrpc_trigger"s. */ @@ -1375,7 +1375,7 @@ ovsdb_jsonrpc_sessions_wait(struct ovs_list *sessions) static void ovsdb_jsonrpc_sessions_close(struct ovs_list *sessions, - const struct ovsdb_jsonrpc_remote *remote) + const void *remote) { struct ovsdb_jsonrpc_session *s, *next; @@ -1390,7 +1390,7 @@ ovsdb_jsonrpc_sessions_close(struct ovs_list *sessions, * reconnect. */ static void ovsdb_jsonrpc_sessions_reconnect(struct ovs_list *sessions, - const struct ovsdb_jsonrpc_remote *remote) + const void *remote) { struct ovsdb_jsonrpc_session *s, *next; @@ -1406,7 +1406,7 @@ ovsdb_jsonrpc_sessions_reconnect(struct ovs_list *sessions, static size_t ovsdb_jsonrpc_sessions_count(const struct ovs_list *sessions, - const struct ovsdb_jsonrpc_remote *remote) + const void *remote) { struct ovsdb_jsonrpc_session *s = NULL; size_t count = 0; @@ -1426,7 +1426,7 @@ ovsdb_jsonrpc_sessions_count(const struct ovs_list *sessions, * re-open the session.) */ static void ovsdb_jsonrpc_sessions_set_options(struct ovs_list *sessions, - const struct ovsdb_jsonrpc_remote *remote, + const void *remote, const struct ovsdb_jsonrpc_options *options) { struct ovsdb_jsonrpc_session *s;
It turns out there is no need for ovsdb_jsonrcp_session to have access to the remote data structure. Make it opaque as a 'void *' pointer. The pointer value is still useful when selecting ovsdb_jsonrpc_sessions that of the same 'remote'. This change will laster multi-threading patches easier because access to 'remote' does not need be protected. Signed-off-by: Andy Zhou <azhou@ovn.org> --- ovsdb/jsonrpc-server.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)