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

[Luiz A. D. de L. <lui...@gm...>]

Hello Chris,

I hope that, if you see my code, it didn't scary you. :-)
I have finished the plugin implementation part and I also almost
finished the ruby-file-sync example. However, I cannot finish the
plugin without a format implementation.

Here comes my problem: formats have no data. There is no
osync_objformat_set_data. I implemented it using the
same strategy that I used to workarround the missing
osync_objtype_sink_get_userdata. It was all ok until I started to
implement the first objformat callback router method. All format
callbacks do not have an OSyncObjFormat argument, as
plugin and objettype sink callbacks have OSyncPlugin and
OSyncObjTypeSink. The only way to define data is by returning some
pointer from initialization function.

ObjFormat seems to follow some logic different from what Sink and
Plugin uses. Take  osync_objformat_new for example. It sets the
objformat name and objtype name and there is no
osync_objformat_set_name. I guess that unifying some behavior would
help the API users.

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
* add a set/get_data to objformat.
* 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?
* Choose one unique data field name. I found three: user_data, userdata and data
* 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.
 * 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?

If some of these are accepted, I can provide some patches.

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


2011/5/25 Luiz Angelo Daros de Luca <lui...@gm...>
>
> Hello Chris,
> I'm sending my ruby plugin. It still misses some important stuff but it is getting there.
> Maybe this can help me to show you exactly what I am doing.
> I am using osyncplugin in order to test it. When I finish the filesync implementation, I will test it with some data.
> RUBYLIB=...libopensync-plugin-ruby-0.39/src /usr/bin/osyncplugin --configdir ~/.opensync/ --config ~/.opensync/group1/2/file-sync.conf --pluginpath ~/prog/opensync/libopensync-plugin-ruby-0.39/build/src/ --plugin ruby-file-sync --initialize
> I still need to implement the "loader part", which looks for ruby files somewhere, some OSyncData stuff and, I hope, file format plugin support.
> I copied the SWIG generated file into src but cmake can generate it in build and compile the module.
> There is a example file in src/example tha might show what a plugin developer will need to write in order to have a working plugin. I also wrote some instructions at the top of this file.
> Now your questions...
>
> 2011/5/23 Chris Frey <cd...@fo...>
>>
>> On Mon, May 23, 2011 at 11:26:55AM -0300, Luiz Angelo Daros de Luca wrote:
>> > Class Info
>> >    def objtype_sinks
>> >         Opensync::osync_objtype_sink_get_objtype_sinks(@_self).collect
>> > {|_sink| Sink.for(_sink) }
>> >    end
>> > end
>> >
>> > It is a little different approach from what I saw in python-module plugin.
>> > All logic is inside ruby world and C code is just for wrapping.
>>
>> Thanks for the explanation and code.  Please keep in mind that I didn't
>> make the original change in opensync to make the _get_ function that you
>> need internal, so I want to make sure I understand all the reasons before
>> I change it.  It's easy to change, but maybe there's a reason behind
>> it that is useful.  I'm trying to maintain opensync, but I didn't write
>> most of it. :-)
>
> I know. We try to do the best we can. The code I'm sending might help on this topic.
>
>>
>> As you say, less code, less bugs... that goes for the library side too. :-)
>>
>> The userdata is basically application data, and it is assumed that the
>> application will keep track of it, and free it, etc.
>
> For every other information (slow_sync, info...), it is opensync choice to show it or not. However, specially userdata,
> it is too much developer choice how to use it. As a newbie in opensync, I might be wrong but I think that opensync may not gain controlling it. It just complicate the developer's world.
>
>>
>> So, I hope this is my last question. :-)  If you just get a list
>> of objtype_sinks with the code above, then when you need to register
>> a callback, how do you know which sink to use?
>>
>
> The code will show but I'll try to explain. In Ruby, I have access to all parameters that the callbacks receive, including PluginInfo. In plugin initialization callback (already inside ruby), I ask for sinks and set my callback functions for each of them. My set_xxx_func receives the sink pointer and the ruby callback. It just save this information inside the sink userdata. Also, it defines a C world callback for it. As a C callback receives sink and even userdata, it is easy to get ruby callback reference and invoke it. The hard part is just the set_xxx_func that has no access to userdata.
>
>>
>> I don't see where you're connecting the sink and the callback in your
>> code.  And if you have a list of sinks already, maybe it would be wise
>> to store your userdata along with it?
>>
>> i.e. If you have a Sink object that contains your sink pointer, and
>> the ruby programmer asks to register a callback via some Sink object
>> method, why wouldn't the userdata be stored in Sink itself?
>
> I changed some of this code. userdata now contains only the callback functions and a single ruby data. There is no reference to sink itself.
>
>>
>> I'm leaning toward exposing osync_objtype_sink_get_userdata() anyway,
>> I just want to fully understand the change.
>>
>>
>> > Sorry, I guess I wasn't clear. I just asked why the name of
>> > osync_objtype_sink_set_userdata is this and not osync_objtype_sink_set_data
>> > like in osync_plugin_set_data. I was worried that
>> > osync_objtype_sink_set_userdata might
>> > be designed to store something other than a pointer for plugin developer own
>> > use. If it is just like what osync_plugin_set_data is for plugins but for
>> > sinks, I'm fine.
>>
>> Yep, both are just void* pointers that opensync stores for the app's use.
>>
>> Maybe a rename is in order before 0.40. :-)
>>
> It is good to know :-)
> BTW, while writing a ruby version of file-sync-plugin, I found some strange things that might be bugs:
> 1) It unref OsyncError on methods that does not own error pointer (i.e. get_sync_info). I read somewhere that this is not what it is supposed to happen.
> 2) plugin data (a.k.a. OSyncFileEnv) has a field named directories that should store sink userdata. However, it is never defined (always null) and sink's userdata might never go. Memory leak?
> 3) all methods that use the filename passes it to filename_scape_characters except osync_filesync_read that uses it directly. Is it expected?
> Thanks,
> ---
>      Luiz Angelo Daros de Luca, Me.
>             lui...@gm...