Re: [Opensync-devel] [trunk] OSYNC_START_TYPE_EXTERNAL example / OSyncClient API changes

[Graham C. <g+o...@co...>]

Just a quick email reply for now -- hopefully I will get some time to actually 
work on this later today.  

On Sunday 01 November 2009 13:32:57 Daniel Gollub wrote:
> On Sunday 01 November 2009 01:39:24 am Graham Cobb wrote:
> > I have a first cut of the external plugin config file ready to check in
> > (http://opensync.pastebin.com/m38d98a95).
>
> Thanks, will review your patch ASAP!

Thanks for your feedback, including in your other emails.

> > 1) When I run the unittests, 17 of them fail.  I don't think any of them
> >  are  related to these changes but is that expected at the moment?

> > If it is not expected I will have to do some more checking.
>
> Would be great if you could check whats going wrong there ...
> Those tests also fail if you revert your changes - right?

That is the first step in the "more checking" :-)  I will look into this later 
this evening.

> > 2) I have created an example external plugin config file in
> > docs/examples/plugins/src.  Can the existing CMake macros be used to get
> >  that  file installed into the plugin directory (which is where I am
> >  looking for it -- I look for .xml files first, then .so files)?  I
> > haven't tried it but I assume the macros like OPENSYNC_PLUGIN_INSTALL
> > expect a .so file.

> I'll introduce a cmake macro called "OPENSYNC_EXTERNAL_PLUGIN_INSTALL" ...

Thanks, I will use that.

> > 3) The XML format includes <ExternalCommand> but I currently ignore
> >  it.  My  plan is that this should be saved in the plugin structure and
> >  could be used as a default for the config option that Henrik is adding.
>
> Ok, fine with me.

I saw your other email on this.  I will try to get to this today but may not 
have time.  I want to concentrate on getting the basic code working and 
tested.

> > 4) If the .xml and the .so are both present, they both get loaded and two
> > plugins exist with the same name.  I thought of adding error checking to
> > osync_plugin_set_name to fail if a plugin of the same name is already
> >  loaded.   But I have just noticed that none of the osync_plugin_set_...
> >  calls have OSyncError parameters.  Should they?  This would be an API
> >  change (to a widely used API).

> Anyway, this is a different issue - independent of your planned changes ..
> which should get fixed at some point. I'll create for that a ticket ... in
> my opionoen it's not a blocker, since this could be avoided by distribution
> with sane packaging ;)

OK.  I am not going to worry about this at the moment.

From your other email:

> Is the drop of osync_module_unload() intended?

Yes, it is necessary.  The pseudo-plugin created by the new external plugin 
code has no module->module (it is NULL).  That seems reasonable to me as no 
module has been loaded.  The only place that gets upset by this is 
osync_module_unload() which asserts that module->module exists.  That also 
seems reasonable to me: you shouldn't be trying to unload a module if one was 
never loaded.

Fortunately, osync_module_unref() already unloads the module if this is the 
last reference and if module->module is non-null (i.e. a module was actually 
loaded).  So, it is never necessary to call osync_module_unload() 
explicitly -- it will be unloaded automatically when the last reference is 
dropped.

In fact, I would even argue it is a bug: the caller does not know that someone 
else does not hold a reference and the module should not be unloaded until 
the last reference is dropped.

So, yes it is deliberate.  The change to the comment was supposed to give a 
hint!

As for ignoring the error on loading the external plugin, you are right that I 
did it because that is the way the existing plugin code worked.  You are also 
right that we should probably be making the error visible somewhere other 
than the trace files and if we *really* want to ignore it we should be 
freeing the error struct.  I will add the FIXME.

You mention that you will write an XSD (thanks -- another thing I have never 
done!).  Should I validiate the XML file against the XSD before loading it?  
I noticed that some other code (can't remember what, offhand, maybe config 
file loading?) does that.

Graham