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

[Tyler Dauwalder <tyler@xxxxxxxxxxxxx>]

On 2003-06-07 at 10:14:10 [-0700], Ingo Weinhold wrote:
> 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.

Okay, then leave them in. :-)

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

That's a tough call... You're right that the DiskDeviceTypes.h names 
are going to be no fun to type... I guess it'd be all right to have 
them both. But do we want Name() + DescriptiveName() or ShortName() + 
Name() or ModuleName() + Name(), or...?

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

Okay.

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

Sounds like SetDescription()/Description() it is. Although, one could 
argue for eliminating SetDescription() and moving the parameter into 
the constructor, particularly since we're already generating a fresh id 
then anyway.

> 
> > Maybe a
> > KDiskDeviceJob::GetInfo(user_disk_device_job_info*) would be handy?
> 
> Certainly not a bad idea. :-)

Let's add it then. :-)

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

How about "completed_subtasks"? That sounds better to me, even though 
it's a little longer.

>     float    current_subtask_progress;
>     char    current_subtask_description[256];
> };

Okay, that's probably a good idea. Let's do that.

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

Okay, either way. :-)

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

Okay, let's do that then.

-Tyler