Re: using libmagic in Emacs?

[Ken Raeburn]

On Aug 22, 2009, at 16:18, address@hidden wrote:
> Andreas Schwab <address@hidden> writes:
> address@hidden writes:
> > > +  GCPRO6 (file_description, file_mime, file_encoding, rv,  
> > > absname, encoded_absname);
> >
> > That's too much.  You only need to protect variables used around  
> > calls
> > that can GC.  Arguments to lisp functions are implicitly  
> > protected.  For
> > example, there are no function calls during the lifetime of absname.
> > And encoded_absname is completely unused.
>
> It seems to me I only need to protect f, which I would do by GCPRO:ing
> absname. Since this is aparently wrong, I will leave it like it is,
> since it doesnt hurt to GCPRO too much. (?)

If ENCODE_FILE returns a new lisp string object, you need to GCPRO  
that object, not absname.  After the call to ENCODE_FILE, absname is  
unused, so it won't need protection.  In fact, it looks to me like  
after that point, GC isn't possible, so I'm not sure anything needs  
GCPRO'tection here.

> > > + libmagic_error:
> > > +  report_file_error("Libmagic error",Qnil);
> > > +  if (cookie != NULL) magic_close (cookie);
> >
> > report_file_error throws, so you leak a resource.
>
> Fixed I think.

No, you're still trying to call magic_close after report_file_error  
returns, which it won't.

> +{
> +  CHECK_STRING (filename);
> +  magic_t cookie=NULL;
> +  char* f = NULL;

CHECK_STRING is executable code, and should be moved down after the  
variable declarations.

> +  const char* rvs;
> +  Lisp_Object file_description;
> +  Lisp_Object file_mime;
> +  Lisp_Object file_encoding;
> +  Lisp_Object rv;
> +
> +  Lisp_Object absname, encoded_absname;
> +  struct gcpro gcpro1, gcpro2, gcpro3, gcpro4, gcpro5, gcpro6;
> +
> +  GCPRO6 (file_description, file_mime, file_encoding, rv, absname,  
> encoded_absname);

It seems to be common -- I'm not sure if it's required, but I would  
conservatively assume so -- for any local variable GCPRO'd to get  
initialized before any possible call to garbage collection, so the  
precise(?) garbage collector won't be scanning random stack values and  
thinking they're lisp objects; if the initialization is unclear, often  
that means simply assigning Qnil just before or after the GCPRO call.

Different GC marking strategies are used on different platforms, so  
the lack of an obvious problem on one platform doesn't mean the code  
will work okay on another.

Ken