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

[Mohammad-Reza Nabipoor]

Hi, Jose.

Thanks for your review :)


On Fri, Dec 18, 2020 at 05:49:06PM +0100, Jose E. Marchesi wrote:
> 
> > /* 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 :)


Yes!
And how much it simplifies the calculations!
Thank you and the Rabbit Herd!


> 
> > 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 :)
> 


I think as you are the writer of the other pickles, your taste matters
for consistency.


> ...
> >     offset<uint<64>,RedoxFS_BLKSZ> parent;
> >     offset<uint<64>,RedoxFS_BLKSZ> next;
> 
> Why not using less verbose standard types?  Like uint16, uint32, etc?


This goes back to my early days of Poke :)
I'll update them.


> 
> >     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?


Sure!
`RedoxFS_Extent[#RedoxFS_BLKSZ - 288#B]` is much more readable.
BTW `RedoxFS_Extent[#RedoxFS_BLKSZ - OFFSET]` does not work as expected.
Did I miss anything?


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


OK.


> 
> >
> > 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";


OK.


> ...
> >     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?  ...


This goes back to when Poke was not capable of having long arrays.
These days, it can.
But I thought it's not useful.
Maybe it's useful in writitng headers to disks.
I'll de-comment it. Thanks.


> Also, remember again you can define array types with sizes, like:
> 
>   uint<8>[#RedoxFS_BlkSZ - 56#B]


Yes. This is nicer.


> > 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?


It is better, but the `reverse` argument is a blocker (I think I can
get rid of it, `reverse` is only useful when all `extents` including the
empty ones get printed).


> >
> > 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?


I cannot. Because of this line `n = RedoxFS_Node @ nxt;` below:


> >     while (nxt != 0#B)
> >       {
> >         n = RedoxFS_Node @ nxt;

Here.

> >
> > 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?
> 

Ditto.


Cheers,
Mohammad-Reza