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

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Hello,

I was and I'll be "offline" until weekend but lets answer and ask
somethings...

---
     Luiz Angelo Daros de Luca, Me.
            lui...@gm...

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 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)

          {
          ...

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. It is a lost battle to try to save the
programmer from its own mistakes. :-)

>
> > * 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.

>
>
> > * 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?

>
>
> > * Choose one unique data field name. I found three: user_data, userdata
> and data
>
> I'd go with userdata, since that's self-documenting.  Patches welcome.
>
>
I'll try to do some coding on weekend.

>
> > * Plugin callback define methods in plugin
> > (set_initialize/finalize/discovery) does not have a suffix like in
> > sink and others. Maybe a osync_plugin_set_initialize_func would sound
> > better.
>
> Sounds good to me.  Patches welcome.
>

I'll try to do some coding on weekend.

>
> >  * Is there any difference in osync_format and osync_objformat? I know
> > that there is only osync_format_env but this format isn't really
> > objformat?
>
> I think the goal was to somehow link the naming of objformat with
> objtype, instead of just the plain "format" and "type".  There are
> a few top level objtypes, but there can be many objformats for each
> objtype.
>
> osync_format_ functions appear to be the namespace used for conversions.
>
> As I understanding, there are objtypes, each objtype can have multiple
> objformats, and then to make use of them, you can create format_env
> environments with which to convert between objformats.
>
> I think the naming is ok here... maybe not consistent from a function
> naming perspective, but from a meaning perspective, it helps to
> remember the hierarchy of types.
>
> Let me know if you disagree. :-)
>

I looked the code and it seems that this is related to dynamic loading of
format modules (which contains objformats and converters) and keeping the
registry of it. Maybe the format-module name that is not sounding OK. It is
not objformat and not converter. It deals with both.

>
> Thanks!
>

Thanks also!

> - Chris
>
>
>
> ------------------------------------------------------------------------------
> vRanger cuts backup time in half-while increasing security.
> With the market-leading solution for virtual backup and recovery,
> you get blazing-fast, flexible, and affordable data protection.
> Download your free trial now.
> http://p.sf.net/sfu/quest-d2dcopy1
> _______________________________________________
> Opensync-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/opensync-devel
>