Re: [Opensync-devel] osync_objtype_sink_get_userdata intentionally not exported?

[Chris F. <cd...@fo...>]

On Wed, Jun 01, 2011 at 01:53:38AM -0300, Luiz Angelo Daros de Luca wrote:
> Hello,
> 
> I was and I'll be "offline" until weekend but lets answer and ask
> somethings...

No problem :-)


> 2011/5/27 Chris Frey <cd...@fo...>
> 
> > On Fri, May 27, 2011 at 01:24:28AM -0300, Luiz Angelo Daros de Luca wrote:
> > > So, I have some suggestions:
> > >
> > > * make all _new function behave the same: only malloc or malloc and
> > > also define mandatory attributes.
> > > * if _new only mallocs, create the necessary setters and getters for
> > > each of the attributes
> >
> > I'm not entirely sure that objformat's name is supposed to change.
> > These names show up in the osynctool --listformats output.
> >
> > I favour APIs that make it impossible to do the wrong thing, and
> > I suspect that this _new(), with attributes, is doing that.
> >
> 
> Sure, my suggestion is just to unify some behavior. Why plugin has a
> set_name and format doesn't?
> Remove from plugin or add it to format. That is my idea.

I'm guessing because plugin had the longname and description to worry about,
and putting that in one function was probably unwieldy.


> I'm just afraid that mandatory parameters result in something like these:
> 
> plugin/opensync_objtype_sink.c
> OSyncObjTypeSink *osync_objtype_main_sink_new(OSyncError **error)
> {
>     return osync_objtype_sink_new(NULL, error);
> }
> 
> OSyncObjTypeSink *osync_objtype_sink_new(const char *objtype, OSyncError
> **error)
> 
>           {
>           ...

The main sink is a special case of regular sinks.  It appears to be
stored as a special pointer in the OSyncPluginInfo struct.  I think it is just
an implementation detail that it reuses the regular sink code.

I wish there was more documentation on what main sinks do.  I'm not
sure I understand your objection to the above code.  To me, it is
clearer to have a function name associated with a null objtype name.
Otherwise, we wouldn't know that this was a special case.


> IMHO, I prefer smaller methods that do a single step and which the arguments
> order follows some kind of general pattern.
> For example. when I compare:
> 
>         plugin = osync_plugin_new(error)
> osync_plugin_set_name(plugin, "file-sync");
>  osync_plugin_set_longname(plugin, "File Synchronization Plugin");
> osync_plugin_set_description(plugin, "Plugin to synchronize files on the
> local filesystem");
>         osync_plugin_env_register_plugin(env, plugin, error)
> 
>   and:
> 
>         plugin = osync_plugin_new("file-sync", "File Synchronization
> Plugin", "Plugin to synchronize files on the local filesystem",error);
>         osync_plugin_env_register_plugin(env, plugin, error)
> 
> Although the first uses many more code, it explains by itself while the
> second case, I would need to know the parameters order to infer what is
> what. The real problem is that what would be accessible by a setter/getter
> method in plugin is not valid for objformat. I like the rule that the
> "programmer" know what to do.

I like code that describes itself too, but that's not the only consideration.
I think there is something more going on here (see comments on
set/get_data below).


> It is a lost battle to try to save the programmer from its own mistakes. :-)

I strongly disagree with this, as programmers need all the help they
can get. :-)  And saving myself from my own mistakes has helped me more
than I can remember, especially as code gets larger, and my brain gets
older.


> > > * add a set/get_data to objformat.
> >
> > I can see adding a get_userdata(), but the set_userdata() is handled
> > by the initialize callback.  By adding set_userdata(), we'd introduce
> > a potential race condition during setup, depending which was called
> > first.
> >
> 
> This case happens in plugin. Is "race condition" about threadsafeness? Is
> opensync design thread-safe? If is just about a double set_userdata, in my
> case, I just return the same user_data that I previously saved. Another
> question: should plugin intialize be only about allocationg userdata or it
> should also set sinks (like in file-sync).
> 
> I'm getting into a point that I think userdata is not enough for my userdata
> needs. I think that I'll need to use my "hacked userdata" for plugin, sink
> and objtype. I need to have a struct to store callbacks and I cannot wait
> until a initialize callback is called in order to allocate this struct.

In reading through the code, it would appear that even the plugin's
set/get_data functions do not do what I thought they did.

It looks like it is consistent: an initialize callback returns its own
user data.  This works for plugins and formats.  The data returned by
the initialize function is passed into the other callbacks, including
the finalize callback.  The data in osync_plugin_set_data() is not.

The plugin data set by osync_plugin_set_data() is never passed back to
any callbacks.  (I did a compile test to make sure... let me know if
you find out differently.)

The data set by osync_objtype_sink_set_userdata() is passed back to the
callbacks set by osync_objtype_sink_set_connect_func() and friends.
These callbacks do not have initialize/finalize associated with them,
since they are setting the sink config for the plugin, not the objformat.
See the linking of plugins and objformats in OSyncPluginInfo.

And similar to the plugin, the objformat user_data defined in
struct OSyncObjFormat is set by the objformat's initialize callback,
and is passed into _its_ callbacks, including finalize.  So far, there
is no get or set for this user_data, and there is no code that needs
access to this user_data except the callbacks themselves, and they have
it in the arguments.

So OpenSync _is_ consistent... just not our understanding of it. :-)



> > > * Some callbacks returns values while others not. Why return
> > > osync_bool if error is enough to tell if the function
> > > worked? Why force initialization to return a void* if, maybe, the
> > > initialization is not used to create a struct for data?
> >
> > If you could point out concrete examples, we could make sure.
> > I'm all for simplifying the API, but maybe the library checks the
> > return values on some of these.  We'd have to reverse engineer the
> > reasoning from the code. :-)
> >
> 
> About the initialize void*, it is a suggestion to let set_data do the job
> and about the bool return,
> it is just redundancy to return OK/FAIL and also error. Should error be
> enough to tell that it failed?

Our understanding of the void* return was faulty... it is not obvious to
me that the void* return is unique... there is no set_data() equivalent.


Thanks for the questions... it forced me to probe a little deeper into
the code. :-)

- Chris