poke-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC] redoxfs.pk: Pickle for RedoxFS file system


From: Jose E. Marchesi
Subject: Re: [RFC] redoxfs.pk: Pickle for RedoxFS file system
Date: Fri, 18 Dec 2020 17:49:06 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)

Hi Mohamad.

> This pickle `redoxfs.pk` needs your review!
> It needs more systematic tests to make sure I didn't miss anything.
>
> The final goal is to provide all facilities to put data in the FS or
> get data from it:
>   - ls
>   - ar a
>   - ar x
>   - find
>   - ...

Very nice! :)

I have some comments from a first quick glance at the pickle.

> /* Block size */
> unit RedoxFS_BLKSZ = 4096UL * 8;  /* `4096*8` bits */

I'm glad to see that user-defined units are now proved to be useful :)

> type RedoxFS_Extent =
>   struct
>   {
>     offset<uint<64>,RedoxFS_BLKSZ> block;
>     offset<uint<64>,B> length;
>
>     method empty = int:
>       {
>         return block == 0#B && length == 0#B;  /* CHKME block ?= 0#B */
>       }

Like we do in C, I would use the convention of using _p for Poke
functions and methods that return truth values, i.e. is_empty_p.

>     method data = uint<8>[]:
>       {
>         return uint<8>[length] @ block;
>       }

I think it is good for methods to have names like DO_WHATEVER.  This
eases them to not be confused with fields.  In this case I would use
something like get_data.

This is a matter of taste anyway and this your pickle, so these are just
suggestions :)

>   };
>
> var RedoxFS_MODE_TYPE    = 0xF000UH,
>     RedoxFS_MODE_FILE    = 0x8000UH,
>     RedoxFS_MODE_DIR     = 0x4000UH,
>     RedoxFS_MODE_SYMLINK = 0xA000UH;
>
> var RedoxFS_MODE_PERM  = 0x0FFFUH,
>     RedoxFS_MODE_EXEC  = 0o1UH,
>     RedoxFS_MODE_WRITE = 0o2UH,
>     RedoxFS_MODE_READ  = 0o4UH;
>
> /* A file/folder node */
> type RedoxFS_Node =
>   struct
>   {
>     uint<16> mode;
>     uint<32> uid;
>     uint<32> gid;
>     uint<64> ctime;
>     uint<32> ctime_nsec;
>     uint<64> mtime;
>     uint<32> mtime_nsec;
>     uint<64> atime;
>     uint<32> atime_nsec;
>     uint<8>[226] name;
>     offset<uint<64>,RedoxFS_BLKSZ> parent;
>     offset<uint<64>,RedoxFS_BLKSZ> next;

Why not using less verbose standard types?  Like uint16, uint32, etc?

>     RedoxFS_Extent[(#RedoxFS_BLKSZ/#B - 288UL) / (#RedoxFS_Extent/#B)] 
> extents;

Hmm, how does the spec define the size of this array exactly?  Remember
Poke supports array types whose extension is defined by a size rather
than by number of elements... maybe it would be more clear to define it
that way?

>
>     method isdir = int:
>       {
>         return (mode & RedoxFS_MODE_DIR) == RedoxFS_MODE_DIR;
>       }
>     method isfile = int:
>       {
>         return (mode & RedoxFS_MODE_FILE) == RedoxFS_MODE_FILE;
>       }
>     method issymlink = int:
>       {
>         return (mode & RedoxFS_MODE_SYMLINK) == RedoxFS_MODE_SYMLINK;
>       }
>   };

Again matter of taste: I woul duse is_dir_p, is_file_p and is_symlink_p.

>
> type RedoxFS_Header =
>   struct
>   {
>     uint<8>[8] signature = ['R', 'e', 'd', 'o', 'x', 'F', 'S', 0UB];

Since the array is 0 terminated, I think you can use a string here:

string signature = "RedoxFS";

>     uint<64> version = 4UL;
>
>     /* Disk ID, a 128-bit unique identifier */
>     uint<8>[16] uuid;
>
>     offset<uint<64>,B> size;
>
>     offset<uint<64>,RedoxFS_BLKSZ> root;  /* Block of root node */
>     offset<uint<64>,RedoxFS_BLKSZ> free;  /* Block of free space node */
>
>     /* Padding */
>     /* uint<8>[#RedoxFS_BLKSZ/#B - 56UL]; */

Why is this commented out?  Remember that you can now use the OFFSET
variable (which is only available within struct type definitions) that
contains the current offset wrt the start of the type.

Also, remember again you can define array types with sizes, like:

  uint<8>[#RedoxFS_BlkSZ - 56#B]


>   };
>
>
> /* Node pretty-printer
>  *
>  * Prints the contents of a `RedoxFS_Node` in a readable fashion.
>  * To increase readability, only non-empty extents are get printed before any
>  * other fields. By default, extents are get printed from first one to last
>  * one by default, but this can be changed by passing 1 in `reverse` argument.
>  */
> fun redoxfs_nprint = (RedoxFS_Node n, int reverse = 0) void:

Wouldn't it be better to define this as a method in the RedoxFS_Node
type?

>   {
>     print ("#<\n  extents = [\n");
>
>     var f = reverse ? n.extents'length : 0UL;  /* first */
>     var l = reverse ? 0UL : n.extents'length;  /* last  */
>
>     while (f != l) {
>       if (reverse)
>         --f;
>
>       var e = n.extents[f];
>
>       if (!e.empty)
>         printf ("    .[%u64d]={block=%v, length=%v}\n", f, e.block, e.length);
>
>       if (!reverse)
>         ++f;
>     }
>
>     var mtype =
>       n.issymlink ? "FILE | SYM"
>       : n.isfile ? "FILE"
>       : n.isdir ? "DIR"
>       : "" ;
>
>     printf (
>       "  ],\n  mode=%s | %u12o,\n  uid=%u32d,\n  gid=%u32d,\n  ctime=%u64d (",
>       mtype, n.mode & RedoxFS_MODE_PERM, n.uid, n.gid, n.ctime);
>     ptime (n.ctime);
>     printf ("),\n  ctime_nsec=%u64d,\n  mtime=%u64d (", n.ctime_nsec, 
> n.mtime);
>     ptime (n.mtime);
>     printf ("),\n  mtime_nsec=%u64d,\n  atime=%u64d (", n.mtime_nsec, 
> n.atime);
>     ptime (n.atime);
>     printf (
>       "),\n  atime_nsec=%u64d,\n  name=%s,\n", n.atime_nsec, catos (n.name));
>     printf ("  parent=%v,\n  next=%v,\n>\n", n.parent, n.next);
>   }
>
> fun redoxfs_root = (RedoxFS_Header hdr = RedoxFS_Header @ 0#B) RedoxFS_Node:
>   {
>     return RedoxFS_Node @ hdr.root;
>   }
>
> /* Visits all the extents of a node (extents + next.extents +
>  * next.next.extents + ...).
>  */
>
> type RedoxFS_EVisitor = (RedoxFS_Extent) void;
> fun redoxfs_extents = (RedoxFS_Node n, RedoxFS_EVisitor v) uint<64>:
>   {

Wouldn't it be better to define this as a method in the RedoxFS_Node
type?

>     var c = 0;  /* counter */
>     var nxt = n.next;
>
>     for (e in n.extents where !e.empty)
>       {
>         ++c;
>         v (e);
>       }
>     while (nxt != 0#B)
>       {
>         n = RedoxFS_Node @ nxt;
>         nxt = n.next;
>         for (e in n.extents where !e.empty)
>           {
>             ++c;
>             v (e);
>           }
>       }
>     return c;
>   }
>
> /* File-system walker
>  *
>  * `redoxfs_walk` traverse the file-system hierarchy and call the vistor
>  * callback function for each entiry (file, directory or symlink).
>  *
>  * First argument of visitor `ftype` is `'f'` for files, `'d'` for
>  * directories and `'s'` for symlinks. The next argument is the name of
>  * the entity. The third one is path of parent directory of the entity.
>  * The last argument is an array of extents corresponds to the entity.
>  */
>
> type RedoxFS_Visitor =
>   (char /*ftype*/, string /*name*/, string /*path*/, RedoxFS_Extent[]) void;
>
> fun redoxfs_walk = (RedoxFS_Node n, RedoxFS_Visitor v) void:

Wouldn't it be better to define this as a method in the RedoxFS_Node
type?

>   {
>     fun nothing = (RedoxFS_Extent e) void: {};
>     fun filev = (RedoxFS_Node n, string curdir) void:
>       {
>         /* TODO(mnabipoor) Measure performance of array construction */
>         var es_count = redoxfs_extents (n, nothing);
>         var es = RedoxFS_Extent[es_count] ();
>         var i = 0UL;
>
>         redoxfs_extents (n, lambda (RedoxFS_Extent e) void: { es[i++] = e; });
>         v('f', catos (n.name), curdir, es);
>       }
>     fun visit = (RedoxFS_Node n, string curdir) void:
>       {
>         if (n.issymlink)
>           v('s', catos (n.extents[0].data), curdir, RedoxFS_Extent[] ());
>         else if (n.isfile)
>           filev (n, curdir);
>         else if (n.isdir)
>           {
>             var name = catos (n.name);
>             var path = (curdir == "/" ? "" :  curdir) + "/" + name;
>             var es_count = redoxfs_extents (n, nothing);
>             var es = RedoxFS_Extent[es_count] ();
>             var i = 0UL;
>             fun ev = (RedoxFS_Extent e) void:
>               {
>                 es[i++] = e;
>                 visit (RedoxFS_Node @ e.block, path); 
>               };
>
>             redoxfs_extents (n, ev);
>             v('d', name, curdir, es);
>           }
>       }
>     var nxt = n.next;
>
>     visit (n, "/");
>     while (nxt != 0#B)
>       {
>         n = RedoxFS_Node @ nxt;
>         nxt = n.next;
>         visit (n, "/");
>       }
>   }



reply via email to

[Prev in Thread] Current Thread [Next in Thread]