FreeLists / openbeosstorage / [openbeosstorage] Re: DiskDevice API v2.1

["Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>]

Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx> wrote:
> On Tue, 1 Apr 2003, Tyler Dauwalder wrote:
> > I had the time, so here are some proposed changes for better
> > partition management support. BPartitions would maintain lists of
> > BEmptySpace objects, each of which represents the largest 
> > contiguous
> > space between child BPartitions that could be filled with a new
> > BPartition.
> Mmh, I don't know, if I see the benefits of this. I even think, it is 
> a
> bit confusing to use the BEmptySpace objects to create partitions. At
> least to my intuition empty space is a `non-thing', so encapsulating 
> it in
> a class is somehow... er... strange. I know that it is not uncommon 
> to
> manage free resources (e.g. `free' lists in memory management), but 
> that
> is just to manage information. Just imagine, you have BDiskSpace 
> object,
> call a method on it (CreatePartition()) and suddenly the object is 
> gone.
> If at all, I'd only provide getter methods by BEmptySpace.

I agree with Ingo here.

> > The ValidateXYZ() functions would be used to bounds/error check
> > modifications before actually solidifying said modification with 
> > the
> > corresponding XYZ() function. The BDiskDevice::CommitUpdates()
> > function would then actually perform the device's batched physical
> > updates.
> Maybe it would be better to have CommitUpdates() in BPartition rather 
> than
> in BDiskDevice. If we provided a partition locking, the siblings of 
> the
> affected partition wouldn't need to be locked. Not sure though.

The siblings might have to be updated as well - i.e. if you move a 
partition around, its predecessor might need to update its "next" link.
I am not sure if it's worth the effort to provide partition-granular 
locking - perhaps it's better to be able to add jobs to the current 
update task=3F AFAICT all partition changes have to be sequential anyway.
We just should limit the number of jobs a single partition has 
outstanding (i.e. only one resize change, one movie, etc.).

> > The BDiskSystem class provides info about supported child and 
> > parent
> > systems for each disk system (i.e. intel extended would require an
> > intel parent).
> When introducing such a class, I would make it more complete. 
> Actually,
> those objects could be returned by the add-ons. The class should then 
> have
> ValidateXYZ(BPartition*,...) (implemented by derived classes), which 
> are
> invoked by their BPartition::ValidateXYZ() counterparts. This means, 
> that
> the add-ons for the used systems must be kept loaded while the API is
> used, but that shouldn't harm that much, I think.
> 
> An alternative -- I guess, you envisioned it like that -- is to not 
> make
> the BDiskSystem methods virtual, but let them call into the kernel, 
> where
> the repective module is asked. I don't think, I like that better.

For R1 that doesn't matter much anyway, so we could just leave those 
methods out completely now (and provide a large enough amount of 
reserved virtuals). But since we want to declare that API beta anyway, 
we don't even have to provide the final answer now.
The way I think of partition updating for R2 is that the real work is 
done by a userland app - the kernel will just have to be updated about 
the current state, it would then maintain its internal translation map 
so that the system will continue to work during those changes. That 
translation map will also be written to disk (logged), so that the on-
disk representation can always be translated correctly.
The boot script could check for any outstanding partition changes and 
run the DriveSetup utility if necessary.

To cut a long story short - I still think it makes sense to provide 
complete userland add-ons to implement the functionality found in the 
different partitioning systems.

> > class BPartition {
> > public:
> >     BEmptySpace* EmptySpaceAt(int32 index) const;
> >
> > // I have resize and move separated because move could be 
> > filesystem
> > // independent, whereas resize could not
> I hope, you're right regarding move. The absolute addressing you 
> found in
> ISO9660 disk scares me a bit. Maybe we can ignore that.

Well, it depends on us if we want to support resizing/moving of file 
systems that require absolute block addressing. It would sure be 
possible, but also much more complicated to do. Doing that online 
(while the partition can still be accessed) might be very complicated 
(for both, the kernel and the file system).
The only file system that require this feature is iso9660 - and there, 
resizing/moving doesn't make any sense. But then, I don't have any idea 
about UDF (BTW what's the relation with Mount Rainier if any=3F). If that 
would require absolute blocks, we should go back to the drawing board 
:)

> >     bool CanResize() const;
> >     bool CanMove() const;
> >     status=5Ft ValidateResize(off=5Ft*) const;
> >     status=5Ft ValidateMove(off=5Ft*) const;
> >     status=5Ft Resize(off=5Ft);
> >     status=5Ft Move(off=5Ft);
> >
> >     bool CanEditParameters() const;
> >     status=5Ft GetParameterEditor(
> >            BDiskScannerParameterEditor **editor,
> >            BDiskScannerParameterEditor **parentEditor);
> >
> > // Perhaps we should have something for checking a partition is
> > // large/small enough for initialization with a given system=3F The
> > // parameter editor could alternately check for this.
> I would just add another method in BDiskSystem.

We could just add another file system error code for the 
fs=5Finitialize() function.
Or, we add another function to the FS API that specifically checks for 
this condition (something like "check partition" or "is partition 
suitable" or whatever).

> > public:
> >     int32 CommitUpdates(BMessenger progressMessenger);
> > };
> status=5Ft CommitUpdates(bool synchronously =3D true,
>       BMessenger progressMessenger =3D BMessenger(),
>       BMessage *template =3D NULL);
> 
> :-)

Let's see how that will turn out with my "adding a job later" idea :)

> > class BDiskSystem {
> > public:
> >     bool SupportsParentSystem(const char *system) const;
> >             // True in most cases. NULL =3D=3D raw device.
> >     bool SupportsChildSystem(const char *system) const;
> >             // False for most file systems, true for most partitioning
> >             // systems.
> >
> >     bool IsPartitioningSystem() const;
> >     bool IsFileSystem() const;
> > };
> As said above, I would make them virtual. Moreover, the Supports*
> System()
> should also have a BPartition* parameter. Otherwise partitioning 
> systems
> that allow only a limited number of levels can't give an 
> authoritative
> answer here. The BPartition* parameter could optionally be NULL to 
> ask
> whether it is supported in general.

I think there should be read-only set of classes, and an additional add
-on based set to actually do any changes. This will be only rarely 
used, so it probably doesn't make much sense to clobber the general API 
with it - so I like something BDiskSystem (as a virtual (and abstract) 
base class), but it should contain everything, and not only the check 
operations.
Thoughts=3F

> Furthermore there should be:
> 
>   virtual const char *Name() const;
> 
>   virtual bool SupportsResizing(BPartition *partition) const;
>   virtual bool SupportsResizingChild(BPartition *child) const;
>   virtual bool SupportsMoving(BPartition *partition) const;
>   virtual bool SupportsMovingChild(BPartition *child) const;
> 
>   virtual bool ValidateResize(BPartition*, off=5Ft*) const;
>   virtual bool ValidateMove(BPartition*, off=5Ft*) const;
>   virtual bool ValidateResizeChild(BPartition*, off=5Ft*) const;
>   virtual bool ValidateMoveChild(BPartition*, off=5Ft*) const;
> 
>   virtual bool ValidateCreateChild(BPartition*, off=5Ft*, off=5Ft*) 
> const;
> 
>   virtual bool ValidateInitialize(BPartition*) const;

Looks good. I guess the Validate*() methods should change the values as 
needed=3F
So the user moves the partition to a particular location, and the 
ValidateMove() call will change the values so that that action can 
really be done (and the GUI can be updated directly).

> > class BDiskSystemList {
> > public:
> >     // To be similar to BDiskDeviceList, offering updates as
> >     // systems are added and removed.
> > };
> >
> > class BDiskDeviceRoster {
> > public:
> >     status=5Ft GetDiskSystems(BDiskSystemList *list) const;
> > };
> 
> I think, I would prefer iteration methods instead (GetNext*(), Rewind
> *()).
> Otherwise all add-ons would need to be in memory.

That's also a reason why I want to separate reading and writing in the 
API. The kernel should always be able to read all partitioning systems, 
and therefore needs all modules loaded that handle the partitions in a 
system.

Adios...
   Axel.