Re: please read: storage interface

[Dietmar Maurer <dietmar maurer-it com>]

Michael Meeks wrote:

>         So lets have some criticism :-) firstly, overall I like the
> changes, they seem to be very worthwhile.
>
> --- bonobo-storage.idl  Mon Oct  9 19:00:01 2000
> +++ bonobo-storage-fat.idl      Fri Oct 13 03:21:21 2000
>
>  module Bonobo {
>
> +        const long MASK_CONTENT_TYPE = 1;
> +        const long MASK_SIZE         = 2;
> +        const long MASK_TYPE         = 4;
> +
> +       typedef string ContentType;
> +
> +       struct StorageInfo {
> +               string          name;
> +               unsigne long    type;
>         typo:   unsigned long
>
>         I would call this InfoFields to match the VFS' naming scheme
> also name the 'MASK' stuff more lucidly eg. FIELD_CONTENT_TYPE.

good idea.

>
>
>                 /**
> +                * get_info:
> +                *
> +                * Returns a StorageInfo structure which contains
> +                * the name, content_type and size info.
> +                */
> +               StorageInfo get_info (in long mask)
> +                       raises (IOError);
> +
> +               /**
> +                * set_info:
> +                *
> +                */
> +               void set_info (in StorageInfo info, in long mask)
> +                       raises (IOError);
> +
>
>         These look good; it may be possible to have things that we
> are not allowed to set the permission of, perhaps it is good to add
> NoPermission exceptions here.
>

yes.

>
>                  *
>                  * writes the buffer to this stream.
>                  */
> -               long write (in iobuf buffer)
> +               void write (in iobuf buffer)
>                         raises (NoPermission, IOError);
>
>         I'm less convinced about this, but I've not seen much
> code that deals with short writes correctly and it confuses
> people, better to block as you suggest.
>
> -               /**
> -                * commit:
> -                *
> -                * Commits any pending changes to the Storage
> -                */
> -               void commit ();
>
>         How did we loose this ? an accident I assume.

No. I have removed everything which is not implemented by the
current backends (or is there a backend which supports nested
transactions?) I have also removed the locking code.

> -               /**
> -                * close:
> -                *
> -                * Close the Stream
> -                */
> -               void close ();
>
>         This is made redundant by the Stream object's lifecycle
> management; good.
>
> -               /**
> -                * eos:
> -                *
> -                * End of Stream?
> -                */
> -
> -               boolean eos ();
>
>         The implementations of this function look extremely ugly /
> non-existant. I assume you propose testing eos by a short read.
>
> -               /**
> -                * length:
> -                *
> -                * Returns the length of the stream
> -                */
> -               long length ()
> -                       raises (NotSupported);
>
>         Again we can find out this from your 'stat' like command
> if we need it I suppose.
>
> -               const OpenMode DENY_READ = 4;
> -               const OpenMode DENY_WRITE = 8;
> +               const OpenMode CREATE      = 4;
> +               const OpenMode FAILIFEXIST = 8;
> +               const OpenMode COMPRESSED  = 16; /* try to compress */
>
>         You prolly want an EXCLUSIVE flag of some sort here.

I thought FAILIFEXIST == EXCLUSIVE, or what is the difference.

Maybe adding an APPEND flag is also a good idea.