[Mono-list] Mono.Unix.Magic libmagic wrapper

[Jonathan Pryor]

On Sat, 2006-09-23 at 10:59 -0400, Milosz Tanski wrote:
> The class name of Magic can be defiantly confusing/entertaining for
> the user who stumbles upon it for the first time. The other thing name
> that comes to my mind is FileMagicDatabase.

"Magic" shouldn't be in the name at all -- it's still confusing.

I would suggest using FileTypeDatabase.

> I've updated the bindings with the things you recommended, and I
> integrated it with the build system. I haven't tackled your point #4
> since I wasn't sure how to approach it exactly.

It may not be solvable.  Let's just assume that the magic database is
UTF-8 already...

> Index: class/Mono.Posix/Mono.Unix.Native/LibMagic.cs
> ===================================================================
> --- class/Mono.Posix/Mono.Unix.Native/LibMagic.cs       (revision 0)
> +++ class/Mono.Posix/Mono.Unix.Native/LibMagic.cs       (revision 0)
> @@ -0,0 +1,50 @@
> +using System;
> +using System.Runtime.InteropServices;
> +using Mono.Unix.Native;
> +
> +namespace Mono.Unix.Native
> +{
> +    
> +    [Flags]
> +    public enum MAGIC_FLAGS

1. We shouldn't provide a low-level libmagic wrapper unless we have a
*really* compelling reason to do so.  (Syscall's "compelling reason" is
that it was written first, and not all of the Syscall methods are
wrapped within a higher-level class.  Neither of these apply here.)

2. Public types should be PascalCased.

3. The current policy is that Mono.Unix.Native wraps things within libc
(which "only" limits us to 2208 methods & whatever types they need).  At
present, no other library is wrapped (as far as it's concerned, though
the reality of xsetattr(2)'s presence within libc.so vs. libxattr.so is
open to debate...).

> Index: class/Mono.Posix/Mono.Unix/FileMagicDb.cs
> ===================================================================
> --- class/Mono.Posix/Mono.Unix/FileMagicDb.cs   (revision 0)
> +++ class/Mono.Posix/Mono.Unix/FileMagicDb.cs   (revision 0)
> @@ -0,0 +1,163 @@
> +using System;
> +using System.IO;
> +using System.Runtime.InteropServices;
> +using Mono.Unix.Native;
> +
> +namespace Mono.Unix {
> +
> +    public class MagicDbException : Exception

use FileTypeDatabaseException.

> +    {
> +        string _text;
> +        
> +        public MagicDbException(string text) : base()
> +        {
> +            _text = text;
> +        }
> +
> +        override public string ToString()
> +        {
> +            return _text;
> +        }
> +    }
> +
> +    public class FileMagicDb : IDisposable
> +    {

Should be FileTypeDatabase.

> +        IntPtr  _magic = IntPtr.Zero;
> +        bool    _followSymlinks = false;
> +
> +        // open the magic database with default database files
> +        public FileMagicDb(bool ReturnMime)

Do you need to make `ReturnMime' a constructor parameter?

It looks like you could call magic_setflags() to change whether the mime
type is returned -- will this work?  If this works, you could have a
GetMimeType() which would call magic_setflags(MAGIC_MIME) and then call
magic_file().

Alternatively, if the above won't work, creating a FileMimeTypeDatabase
sub-class might be useful, and make this constructor `internal'.

> +        // open the magic database with a custom database file list
> +        public FileMagicDb(bool ReturnMime, string[] dblist)

Should `dblist' be a `params' array?

> +        ~FileMagicDb()
> +        {
> +            if (_magic != IntPtr.Zero) {
> +                LibMagic.magic_close(_magic);
> +                 _magic = IntPtr.Zero;
> +            }
> +        }

Code duplication is bad. :-)

Create a Close() method, and have both the finalizer and Dispose() call
this shared method.

> +        public void AddDefaultDbFiles()
> +        {
> +            if (LibMagic.magic_load(_magic, null) != 0)
> +                throw new MagicDbException(this.Error); 
> +        }

Shouldn't this be called within the default constructor?

Use AddDefaultDatabaseFiles().

> +        public void AddDbFile(string file)
> +        {
> +            if (LibMagic.magic_load(_magic, file) != 0)
> +                throw new MagicDbException(this.Error); 
> +        }

AddDatabaseFile().

> +        public bool FollowSymlinks

FollowSymbolicLinks (consistency with the rest of Mono.Unix).

> +        private string Error

LastError?

> +        public string Lookup(string filename)

GetFileType().

> +        public string Lookup(byte[] data)

GetTypeFromData().

> +        static public string Mime(string filename)

GetDefaultFileMimeType().

> +        {
> +            string mime;
> +            FileMagicDb m = new FileMagicDb(true);
> +
> +            mime =
> Marshal.PtrToStringAuto(LibMagic.magic_file(m._magic, filename)); 
> +            if (mime == null) {
> +                throw new MagicDbException(m.Error);
> +            }
> +
> +            return mime;
> +        }

Is libmagic thread safe?  If it is, you might consider using a static
instance instead of creating it anew every method call.

Also, use UnixMarshal.PtrToStringUnix() instead of
Marshal.PtrToStringAuto().  The latter may not always be UTF-8, and we
should hope that the magic DB will be consistently UTF-8.

> +        static public string Description(string filename)

GetDefaultFileType().

 - Jon