Re: bk commit into 5.2 tree (rafal:1.2611) WL#4063

[Rafal Somla]

Hi Lars,

I went over your other review comments. This is the list of things I fixed in a 
new patch which will follow soon. I also increased size of the item info entry 
to 2 bytes.

>> Other comments/suggestions
>> --------------------------
>> - s/suhc/such/
>>

Done and fixed other spelling errors.

>> - "These functions are used for memory (de)allocation."
>>   s/Module using this library should define them/
>>     Any program using this module, need to have these defined/
>>
>>   The point is that the kernel is dependent on stream which is
>>   dependent on memory allocation.  There is no need that the *kernel*
>>   provide memory, it could be some other module that is linked with   
>> the kernel and stream module .  I think this way of seeing things:
>>   1) Is easier to understand
>>   2) Is more flexible
>>
>>   And, in the same way: "backup kernel" depend on "stream" which
>>   depend on "catalog service interface".  The latter is usually
>>   provided by the kernel, but need not to be (e.g. for exotic
>>   testing, I think you did something like this already?).
>>

Comment removed.

>> - Default block size can probably be at least 32 kb.
>>

Changed to 32 kb.

>> - s/BASE TYPES/BASIC TYPES/
>>

Changed.

>> - s/main parts/main chunks/   (Lets try to minimize number of new
>>   concepts and only use "chunk")
>>

These parts are not chunks: single part can consist of several chunks.

>> - I wonder if the concept set { part, chunk, fragment, block,
>>   preamble, catalog, snapshot, header, fragment header } can be
>>   simplified.
>>

You can give it a try ;)

>> - Why is the name of the storage engine a blob and not a string?   
>> Same question for version "extra".
>>

It is a string stored in a memory area described by a blob (the common type used 
through the library to describe memory areas). Advantage of blob over char* is 
that blob contains a pointer to the end of the string, so we are safe from any 
null termination issues.

>> - Some file like "stream_v1.h" should be the definition of the API
>>   between kernel and stream.  (If we change the image format, to new
>>   version, we might still use the same api).
>>
>> - In the header file, move the simpler type to start of file to make
>>   it easier to read it.
>>

I rearranged the header files a bit.

>> - The code is prepared so that we can have different error codes in
>>   the future.  I think this is were we need to go mid-term, so this is
>>   excellent.
>>
>> - I like to call the atomic parts in the backup "objects".  I've asked
>>   Robin to comment on what he thinks we should call them.  Items are
>>   probably fine though.

If we can live with items, that would save me at least 2hrs of work with 
renaming everything and reformulating all comments/descriptions. If it is really 
important, how about filling a bug report so that we can fix it after I push the 
patch?

>>
>> - I don't think there is a point in having "v1" in the file names.  We
>>   don't know how the image format will evolve and it is easy to rename
>>   files later if needed.
>>

What about this: files which don't contain any "v?" refer to the current version 
of the stream used in the backup kernel, files with "v?" define the indicated 
version of the format. So, in a .c file, if I want to read backup stream in the 
most recent format, I would say:

#include "stream.h"

But if I want to read stream saved in a specific format, e.g. v1, I'll say:

#include "stream_v1.h"

Does that sound reasonable?

Best,
Rafal