FreeLists / openbeosstorage / [openbeosstorage] Re: Kernelland Draft Headers

["Ingo Weinhold" <bonefish@xxxxxxxxxxxxxxx>]

On Fri, 06 Jun 2003 18:03:40 -0700 Tyler Dauwalder <tyler@xxxxxxxxxxxxx
> wrote:
> 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.

This locking -- the term doesn't actually fit that well, I think, since 
it's only a mechanism to ensure the exclusive use of the modification 
methods of the API -- doesn't prevent the kernel structures from being 
changed. E.g. a device might be unplugged or a media ejected at any 
time, and also formerly scheduled jobs might (and probably will) change 
structures (although only those being marked busy). Now, none of these 
changes will interfere with the modification operations, and they are 
not immediately destructive anyway (as they change the shadow 
partitions only), so at least in this context the change counters are 
not needed.

On the other hand, I think, change counters will simplify 
BDiskDevice::Update(), e.g. to find out, if something has changed 
(needed for the `updated' return value). Though in this case it would 
be sufficient to have only one for the disk device.

> (2) I liked the *user_data == BPartition idea.
> 
> (3) I say get rid of it.

Yes, we probably should.

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

OK.

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

OK.

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

Ah, wait a second. I may have confused myself. :-) BPartition::Type()/
ContentType() returns a type defined in DiskDeviceTypes.h, and 
BDiskSystem::Name() certainly does as well. So, KDiskSystem::Name() 
would be for kernel-internal use only, while only 
KDiskSystem::DescriptiveName() (I'd prefer that one) is passed to the 
userland API. 

Does that sound OK, or do we want to have the internal name available 
in the userland API as well? If we don't, I already hear the command 
line fans shouting, because they would need to pass `BFS Filesystem' 
instead of `bfs' to mkfs.

> (2) By which you mean a parameter to pass in the id to be used for 
> the partition, I assume?

Yes.

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

OK. It may even be necessary, since the create_child_partition() 
exported by the disk device manager will also register the partition 
with that ID, so that changing the ID of the partition may be a bit 
painful later.

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

It's gone.

> (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().

On the other hand, a SetDescription()/Description() combination can be 
implemented in the base class, while a virtual GetDescription() would 
need to be added to each subclass. Moreover I think the job description 
shouldn't change after the job is constructed and it may even be, that 
the information to compose the description is not longer available 
after the respective shadow partition has been destroyed.

> Maybe a 
> KDiskDeviceJob::GetInfo(user_disk_device_job_info*) would be handy? 

Certainly not a bad idea. :-)

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

Well, there shouldn't be a problem to use the extra progress info for 
that.

> We could also 
> add progress updates per subtask, I suppose, to allow two status bars 
> to be shown. Is this what you're thinking?

Something like this. What I don't find so nice, if we have only a 
progress value for the complete task, is that the subtasks may be very 
differently fast. E.g. for resizing a partition, where setting the new 
partition will probably be very quick, while resizing the FS may take 
quite some time. So, if a simple progress bar is shown, it appears like 
half the progress is made in a second, whereas the other half takes an 
hour.

It may be nice to have a more a more detailed progress info, like a 
BDiskDeviceJob::GetProgressInfo(disk_devive_progress_info *info) with:

struct disk_device_progress_info {
        int32   subtask_count;
        int32   done_subtasks;
        float   current_subtask_progress;
        char    current_subtask_description[256];
};

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

Or the other way around: Keep it and drop it, if it won't be used. :-) 
Meanwhile I think, it could be nice for the modules to have it, since 
otherwise they had to iterate through all partitions of the parent 
partition to learn about the index.

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

So am I. :-)

> (3) Well, a char[B_PATH_NAME_LENGTH] would certainly be fair, but I 
> think it falls under the same argument as above.

Even worse, it uses 1024 bytes. :-)

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

Agreed. For `parameters' and `content_parameters' can't be fixed size 
array, I tend towards char* for all.

CU, Ingo