Re: DiskDevice API v2.1

On 2003-04-02 at 07:28:29 [-0800], Ingo Weinhold 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. 

The main motivation is that it gives the partition code a chance to 
declare what regions of unused space are valid for creating child 
partitions. 

> I even think, it is 
> a
> bit confusing to use the BEmptySpace objects to create partitions.

This I agree with now that you mention it. :-)

> 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 think I still like the idea of the BEmptySpace class (maybe 
BPartitionableSpace would be better?), but I agree with moving the 
creation functions into the parent partition (i.e. 
CreateChildPartition). Does that sound better?

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

That's okay with me. I think we should have a way of finding out which 
partitions are locked and busy though (i.e. IsLocked()), or would you 
plan on Lock() being non-blocking?

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

Yes, I agree.

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

Why do you prefer the userland version?

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

Will do.

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

Okay.

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

Can do, but now you have to explain to me why you're so fond of 
synchronous operation, because I still don't get it (and if you told me 
back when we were working on the MIME update functions, it obviously 
didn't stick :-). 

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

Very good idea.

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

Sounds good, though should ValidateInitialize include an off_t *size as 
well?

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

Okay.

-Tyler