Re: core segfaults (Yahoo?)

On Thu, 11 Mar 2004, Meredydd wrote:

> On Thursday 11 March 2004 05:38, A. Craig West wrote:
> > On Wed, 10 Mar 2004, A. Craig West wrote:
> > ===================================================================
> > --- core/src/plugin.c	(revision 306)
> > +++ core/src/plugin.c	(working copy)
> > @@ -121,6 +121,18 @@
> >  }
> >
> >  #if HAVE_LIBLTDL  // MOTHBALLED
> > +static eb_plugin * create_plugin(char * fnam)
> > +{
> > +  eb_plugin * plugin = (eb_plugin *)malloc(sizeof(eb_plugin));
> > +  plugin->filename=(char *)strdup(fnam);
> > +  plugin->info=NULL;
> Note this line.
> > +  plugin->status=PLUGIN_NOT_LOADED;
> > +  plugin->errormsg=NULL;
> > +  plugins=e_list_append(plugins, plugin);
> > +
> > +  return plugin;
> > +}
> > +
> >  static void add_error_entry(char * fnam, char * errormsg)
> >  {
> >    eb_plugin * plugin;
> > @@ -128,20 +140,23 @@
> >
> >    if((plugin=plugin_by_name(fnam))==NULL)
> >    {
> > -    plugin=(eb_plugin *)malloc(sizeof(eb_plugin));
> > +    plugin=create_plugin(fnam);
> >      isnew=1;
> >    }
> >
> > -  plugin->filename=fnam;
> > +  free(plugin->info);
> > +  free(plugin->errormsg);
> ...but then, if there's an error, we create the plugin with NULL info 
> and errormsg, and then free them straight away. Huh?

If they are NULL, free does nothing, so we don't lose much. I added these
lines for the possibly imaginary case where we found a plugin with the
correct name in plugin_by_name, to ensure that we don't leak the info and
errormsg that it previously had.

> 
> >    plugin->info=NULL;
> >    plugin->status=PLUGIN_CANNOT_LOAD;
> >    plugin->errormsg=(char *)strdup(errormsg);
> Now that looks better...
> 
> >    if(isnew)
> >    {
> > -    plugins=e_list_append(plugins, plugin);
> Why aren't we adding it any more here?

We add it in create_plugin. I am considering making create_plugin the direct
equivalent to a C++ constructor and pass in all the arguments, so that it can
call the new_plugin_notify method itself. Duplicate code is evil, after all
:-)

> >      new_plugin_notify(plugin);
> > -  } else {
> > +  }
> > +  else
> > +  {
> Personally, I prefer
> } else {
> rather than
> }
> else
> {
> 
> ...your call, in your code, but please don't go changing my formatting 
> on my turf on too regular a basis.

Sorry about that, I just find it inconsistent to do else statements in K&R
style and everything else in Stroustrup style...

> 
> >      plugin_update_notify(plugin);
> >    }
> >  }
> > @@ -154,6 +169,7 @@
> >    dlhandle dat;
> >    eb_plugin_info * info;
> >    eb_plugin * plugin;
> > +  int isnew=0;
> >
> >    full_path=(char *)malloc(strlen(path)+strlen(fnam)+2);
> >    sprintf(full_path, "%s/%s", path, fnam);
> > @@ -192,12 +208,27 @@
> >    // Now, we dispatch based on plugin type. Are we all sitting
> > comfortably?
> >
> >    if(plugin==NULL)
> > -  { plugin=(eb_plugin *)malloc(sizeof(eb_plugin)); }
> > +  {
> > +    plugin=create_plugin(full_path);
> > +    isnew=1;
> > +  }
> >
> > +  free(plugin->info);
> > +  free(plugin->errormsg);
> Once again, we free(NULL) if plugin==NULL...

We free it whether it is NULL or not... I could put it in an else statement,
statement, I suppose...

> 
> >    plugin->status=PLUGIN_LOADED;
> >    plugin->errormsg=NULL;
> >    plugin->info=info;
> >
> > +  if(isnew)
> > +  {
> > +    new_plugin_notify(plugin);
> > +  }
> > +  else
> > +  {
> > +    plugin_update_notify(plugin);
> > +  }
> OK, I can believe that this is necessary (the plugin notify API is 
> pretty vestigial right now, anyhow).

As I said, I am considering moving the new_plugin_notify into the
create_plugin call, but I don't think it will be worth it, so I didn't bother

-- 
Craig West         Ph: (416) 666-1645	|  It's not a bug,
[email protected]              	|  It's a feature...