Re: DiskDevice API v2.1

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.

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

> class BEmptySpace {
> public:
> 	off_t Offset() const;
> 	off_t Size() const;
> 	int32 Index() const;
>
> 	BPartition* Parent() const;
>
> 	bool CanCreatePartition() const;
> 	status_t ValidateCreatePartition(
> 	           off_t *start,
> 	           off_t *size) const;
> 	BPartition* CreatePartition(off_t start, off_t size);
> 		// Partition creation this way would always create an
> 		// unformatted partition. Does this seem reasonable?
> };

As written above, I would drop the *CreatePartition() methods.

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

> 	bool CanResize() const;
> 	bool CanMove() const;
> 	status_t ValidateResize(off_t*) const;
> 	status_t ValidateMove(off_t*) const;
> 	status_t Resize(off_t);
> 	status_t Move(off_t);
>
> 	bool CanEditParameters() const;
> 	status_t GetParameterEditor(
>            BDiskScannerParameterEditor **editor,
>            BDiskScannerParameterEditor **parentEditor);
>
> // Perhaps we should have something for checking a partition is
> // large/small enough for initialization with a given system? The
> // parameter editor could alternately check for this.

I would just add another method in BDiskSystem.

> 	bool CanInitialize() const;
> 	status_t GetInitializationParameterEditor(const char *system,
>                BDiskScannerParameterEditor **editor) const;
> 	status_t Initialize(const char *diskSystem,
> 	                 const char *parameters);
>
> 	bool CanDelete() const;
> 	status_t Delete();
> };
>
> class BDiskDevice {
> public:
> 	int32 CommitUpdates(BMessenger progressMessenger);
> };

status_t CommitUpdates(bool synchronously = true,
	BMessenger progressMessenger = BMessenger(),
	BMessage *template = NULL);

:-)

> class BDiskSystem {
> public:
> 	bool SupportsParentSystem(const char *system) const;
> 		// True in most cases. NULL == 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.

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_t*) const;
  virtual bool ValidateMove(BPartition*, off_t*) const;
  virtual bool ValidateResizeChild(BPartition*, off_t*) const;
  virtual bool ValidateMoveChild(BPartition*, off_t*) const;

  virtual bool ValidateCreateChild(BPartition*, off_t*, off_t*) const;

  virtual bool ValidateInitialize(BPartition*) const;

> class BDiskSystemList {
> public:
> 	// To be similar to BDiskDeviceList, offering updates as
> 	// systems are added and removed.
> };
>
> class BDiskDeviceRoster {
> public:
> 	status_t GetDiskSystems(BDiskSystemList *list) const;
> };

I think, I would prefer iteration methods instead (GetNext*(), Rewind*()).
Otherwise all add-ons would need to be in memory.

CU, Ingo