[openbeosstorage] Kernelland Draft Headers

Here are my header comment/question comments. I skipped over things 
we're already discussing in the other thread, but otherwise I think I 
hit everything.

userland_interface.h
--------------------

#ifndef _DISK_DEVICE_MANAGER_USERLAND_INTERFACE_H
#define _DISK_DEVICE_MANAGER_USERLAND_INTERFACE_H

// userland partition representation
struct user_partition_data {
        int32                           id;
        off_t                           offset;
        off_t                           size;
        int32                           block_size;
        uint32                          status;
        uint32                          flags;
        int32                           change_counter; // needed? (1)
        char                            *name;
        char                            *type;
        char                            *content_type;
        char                            *parameters;
        char                            *content_parameters;
        void                            *user_data;             
                          // Maybe. Depends on BPartition. (2)
        int32                           child_count;
        user_partition_data     *children[1];
};

status_t get_partition_data(int32 partitionID, bool shadow,
                                                        user_partition_data 
*buffer,
                                                        size_t bufferSize, 
size_t *neededSize);
        // Dangerous?! (3)
status_t get_partitionable_spaces(int32 partitionID, bool shadow,
                                                                  
user_partitionable_space_data *buffer,
                                                                  size_t 
bufferSize, size_t *neededSize);
        // Pass the partition change counter? If GetPartitionInfo() is 
        // only allowed, when the device is locked, then we wouldn't need
        // it. (4)

status_t initialize_partition(int32 partition, const char *diskSystem,
                                                          const char 
*parameters);
        // Note: There is also fs_initialize_volume()... (5)

(1) Probably not needed now we have locking.

(2) I liked the *user_data == BPartition idea.

(3) I say get rid of it.

(4) I say require locking. One will need the lock to do anything 
useful anyway. If you really just want an overview of free space, 
lock, get the space, and unlock.

(5) Still in discussion elsewhere :-).


KPartitioningSystem.h
---------------------

class KPartitioningSystem {
        virtual bool SupportsRepairing(KPartition *partition, bool checkOnly,
                                                                   bool 
*whileMounted);
                // Does that makes sense for partitioning systems? (1)
};


(1) I say yes, we may as well keep it, as other partitioning apps on 
other platforms can sometimes mangle things.  

KDiskSystem.h
-------------
class KDiskSystem {
        const char *Name() const;
// We might want to introduce another name -- a more user                       
// friendly one. This one is the name of the partition module/FS add-
// on, e.g. "intel/extended", "bfs". (1)


        virtual status_t CreateChild(KPartition *partition, off_t offset,
                                                                 off_t size, 
const char *parameters,
                                                                 KDiskDeviceJob 
*job, KPartition **child);
        // optional childID parameter or allow setting the ID later? (2)

(1) Yes, I think it'd be nice to have something like this available. 
const char*FriendlyName() const; or something to that degree 
(DescriptiveName() maybe?).

(2) By which you mean a parameter to pass in the id to be used for 
the partition, I assume? If so, I think that makes sense. We'll need 
to give it an id before it percolates up to BPartition::CreateChild() 
anyway; it may as well have a valid id upon construction.

KDiskDeviceJob.h
----------------

class KDiskDeviceJob {
public:
        void SetID(int32 id);
// The base class might simply generate IDs on construction only. (1)

        void SetDescription(const char *description);
        const char *Description() const;
                // Maybe better just a virtual void GetDescription(char*)? (2)

        void SetPartitionID(int32 partitionID);
                // Probably not needed, since passed to the constructor. (3)

        virtual void UpdateProgress(float progress);
// may trigger a notification
// virtual, since some jobs are composed of several tasks (e.g. Move).
// We might want to explicitly support subtasks in the base class, or
// even in the userland API. (4)

};

(1) I think that makes sense.

(2) Well, if the main use of the Description() function is going to 
be to help fill up a user_disk_device_job_info struct, and assuming 
user_disk_device_job_info::description is to remain a static array as 
it currently is, it would allow the KDiskDeviceJob subclass to just 
printf() into the argument passed to GetDescription(). Maybe a 
KDiskDeviceJob::GetInfo(user_disk_device_job_info*) would be handy? 

(3) Agreed. :-)

(4) This is kind what I invisioned the extra progress info updates 
being used for, but perhaps more simply so, i.e. a way to notify the 
user at what stage of a given task the process was at. We could also 
add progress updates per subtask, I suppose, to allow two status bars 
to be shown. Is this what you're thinking?

disk_device_manager.h
---------------------

// C API partition representation
typedef struct partition_data {
        int32   index;                  // needed? (1)
        uint32  status;
        uint32  flags;
        char    name[B_FILE_NAME_LENGTH];                       // better char* 
? (2)
        char    type[B_FILE_NAME_LENGTH];                       //
        char    content_type[B_FILE_NAME_LENGTH];       //
} partition_data;

// C API disk device representation
typedef struct disk_device_data {
        char    *path;                  // a char[] ? (3)
} disk_device_data;

(1) Probably not. Should be easy to resurrect if we take it out now 
and need it later. :-)

(2) Well, it does seem like a lot of the 256 characters in each of 
those fields is going to go wasted. On the other hand, it would 
complicate things slightly. I'm pretty much indifferent, at this 
point...

(3) Well, a char[B_PATH_NAME_LENGTH] would certainly be fair, but I 
think it falls under the same argument as above. Unless anyone else 
has some convincing arguments otherwise, I think I'd vote for making 
all the character string parameters either char* or char[], leaning 
towards char*.

-Tyler

Other related posts: