Re: [Patch] Added a JFFS2 pickle

[Jose E. Marchesi]

Hi Matt.
Thanks for the new version of the patch!

>>I think in situations like this it is better to have a method to return
>>the offset of data, `get_data_offset', than a method that maps an array
>>of bytes.
>>
>>The reason is that `data' is a payload in this case, i.e. it can contain
>>all sort of things, unrestricted.  Therefore, the user will most
>>probably want to map stuff there: maybe an array of bytes yes, but also
>>maybe an JPEG file.
>>
>>If you give the user an array of bytes and she is interested in mapping
>>something else in that area, she will just use get_data'offset and
>>get_data'size as coordinates for mapping, and the array of bytes will be
>>mapped for nothing.
>
> Ok I like that. Letting the user map the data as whatever they want does
> seem to be the more poke'ish way of doing things.
>
> I can add the get_data_offset function, but I wonder if it would
> make more sense to just have the user do inode.data'offset instead?
> What do you think best practice is there?
>
> I thought about getting rid of the data field, but it's the only
> field in the struct that has a 'offset attribute, so I can't get
> a global offset of the data without it.

With a get_data_offset you don't need a `byte[0] data' field.

get_data_offset will return a relative offset of `data' in the struct.
If the struct is mapped, the user can do:

thing'offset + thing.get_data_offset

to get a global offset.  If the struct is not mapped, the relative
offset may also be useful.

>>> +/*
>>> + * Holds a single extended attribute name/value pair
>>> + *
>>> + * Associated to a file (ino) via an XRef node.
>>> + * This allows XAttr nodes to be reused by multiple files
>>> + */
>>> +type JFFS2_XAttr =
>>> +struct
>>> +{
>>> +    uint<16> magic : magic in [JFFS2_MAGIC, JFFS2_MAGIC_OLD];
>>> +    uint<16> node_type = JFFS2_NODETYPE_XATTR;
>>> +    offset<uint<32>, B> total_len : total_len >= JFFS2_HEADER_SIZE;
>>> +    uint<32> node_header_crc;
>>> +    uint<32> xid;
>>> +    uint<32> version;
>>> +    uint<8> xprefix;
>>> +    offset<uint<8>, B> name_len;
>>> +    offset<uint<16>, B> value_len;
>>> +    uint<32> data_crc;
>>> +    uint<32> node_crc;
>>> +    char[name_len] name if name_len != 0#B;
>>> +    byte[0] data;
>>> +    byte[0] data_end @ total_len;
>>
>>Why data_end needs to be a named field?
>
> Will fix!
>
>>> +    // padding
>>> +    byte[0] @ total_len + alignto(total_len, JFFS2_ALIGNMENT);
>>> +
>>> +    method get_name = string:
>>> +    {
>>> +        if (name_len != 0#B) {
>>> +            return catos(name);
>>> +        } else {
>>> +            return "";
>>> +        }
>>> +    }
>>
>>catos does the right thing with empty arrays of characters:
>>
>>(poke) var a = char[]()
>>(poke) a
>>[]
>>(poke) catos (a)
>>""
>>
>>So I think you can simplify that.
>
> I think you may have missed the name field here:
>
> +    char[name_len] name if name_len != 0#B;
>
> Per your suggestion in the last review I made this an optional
> field contingen on name_len. That's why I had to add the
> if...else here.

Oh, yes you are right :)

I think you can avoid replicating the optional logic by using the usual
try return catos (name); if E_elem { return ""; }

>>Also, you may want to add set_name methods too, using stoca.  This is
>>an example from id3v1.pk:
>>
>> method set_comment = (string val) void:
>> {
>>   try stoca (val, data.comment, ' ');
>>   catch if E_elem { stoca (val, data.extended.comment, ' '); }
>> }
>
> I thought about adding setters for this and also for dirent names, but
> I came to the conclusion that it wouldn't be very useful as
> the name field is not of fixed length, so changing it could change
> the size of the node.

Ah I see.

> The correct way to make changes to any file in a JFFS2 file system
> is to create a new node with an incremented version field at the tail
> of the file system. Doing that requires some knowledge of the physical
> attributes of the flash chip that the filesystem was intended to be
> placed on, namely the eraseblock size and page size. In the future,
> I could make those values (optional?) struct arguments and
> add some helper functions for creating/adding nodes. So unless
> you object, I think I would leave that feature for a jffs2
> pickle 2.0.

Functions to add new nodes seem useful!  They can get these attributes
as arguments :)

Yes sure, you can add these in the future.

>>> +type JFFS2_Summary =
>>> +struct
>>> +{
>>> +    uint<16> magic : magic in [JFFS2_MAGIC, JFFS2_MAGIC_OLD];
>>> +    uint<16> node_type = JFFS2_NODETYPE_SUMMARY;
>>> +    offset<uint<32>, B> total_len : total_len >= JFFS2_HEADER_SIZE;
>>> +    uint<32> node_header_crc;
>>> +    uint<32> sum_num;
>>> +    uint<32> cln_mkr;
>>> +    uint<32> padded;
>>> +    uint<32> sum_crc;
>>> +    uint<32> node_crc;
>>> +    byte[0] records; // This will be large. Don't want to map it unless 
>>> needed
>>> +    byte[0] records_end @ total_len;
>>
>>Why having records and records_end?
>>I would just have a get_records_offset method.
>
> A "get_records_offset" method could be useful, so I'll add that. But I also
> think that the "get_records" method is useful, since it's not likely the
> user will want to map the record data as anything other than an array of
> summary records.

Yes makes sense.  I thought it was some sort of payload.