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

[Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx>]

On Thu, 27 Mar 2003, Tyler Dauwalder wrote:

> On 2003-03-25 at 14:50:51 [-0800], Ingo Weinhold wrote:
[...]
> > In principle I like the idea of a base class for managing the
> > `contiguous
> > disk space' aspects, though I'm not yet sure, if it would perhaps be
> > better to introduce a third class, from which both BPartition and
> > BDiskDevice are derived from. A couple of BPartition methods are
> > simply
> > not applicable -- Index(),
>
> Doesn't do any harm though. Device.Index() == 0 makes at least a
> marginal amount of sense, similar to a single primary partition in an
> intel map.
>
> > Parent(),
>
> Device.Parent() == NULL lets you know when you've gotten to the bottom
> of the tree, though, similar to BDirectory("/").GetParent(...) ==
> B_ENTRY_NOT_FOUND. I think that's actually useful.
>
> > {Minimum,Maximum}Size() (well,
> > they
> > could be computed, but make little sense),
> > MoveTo(), ResizeTo().
>
> I guess I looked at it as being no different than a partition that
> didn't support resizing or moving.

OK, I see, when taking a favorable view of the meaning of these methods
wrt. BDiskDevice, one can even find some useful semantics. ;-) In any
case, they don't do any harm at least.

> > On the other hand that would probably introduce more need for casting
> > and
> > make the API less convenient to use.
>
> That's my main complaint.

I also think, that the disadvantages would be predominant.

> > [...]
> > > Aside from stripping out all the BSession related stuff, I also made
> > > the partition management code a little more specific. It seemed to
> > > me
> > > that the method we were using for partitioning was pretty general,
> > > i.e. for every partitioning system we support, we'd have to write a
> > > new set of parameter editors to support the entire partitioning
> > > process (correct me if I'm wrong here).
> >
> > That's right. But in fact, the two partitioning systems supported by
> > BeOS
> > were different enough to justify separate editors, in my opinion.
>
> Hmmm, I see. This is where we differ, then. :-) My reaction to the two
> partitioning editors has been "why are there two of them with such
> notably different interfaces, when they aren't really that different?".
>  :-)

I don't think, we differ that much wrt. the editors. Their GUI could
indeed be more uniform. But obviously the implementor of the intel DS
add-on wanted to safe some work/time anyway, leaving out support for
extended partitions.

Regarding the partition systems, well, I think, they really differ quite a
bit. The apple system seems to support no hierarchy (?) but an arbitrary
(?) number of top-level partitions, while intel style has a four top-level
partition limit and two levels of hierarchy. Sure, in both cases you can
use a tree view to display the partition structure and common properties,
but the GUI needs to behave differently. E.g. for intel style, if you have
an `Add Partition' button, it should be disabled, when there're already
four top-level partitions. And actually the power user would want to also
see empty partition table slots (or the indexing of the primary
partitions) to have full control -- don't know how PartitionMagic handles
that.

That reminds me: How would you want to treat empty partition table slots
in the API? In the previous one they were represented as partitions being
marked `empty'. Instead Index() could return the actual index. I'm not
sure though, if that would be confusing, since it could differ from the
index of the object in the parent's list. Maybe a second method
(ActualIndex())?

> However, after reading your comments and considering it more, I'm
> starting to think it's perhaps not going to be worth the effort when we
> have only 2 partitioning systems to support.

Since it looks like there will be a PPC version, thanks to Axel, it will
at least be two. With the upcoming new partioning system for x86
machines (don't know the name at the moment) there'll be another one.

> Incidentally, I meant to mention that I'm not insisting upon using this
> new partitioning process interface by any means; I thought it was worth
> looking into, and that ditching it for the old interface would be easy
> enough to do if desired.

I think, we should at least have a closer look. Maybe we find an elegant
solution.

> > > I'd like to see our
> > > DriveSetup replacement offer a nice, unified UI for partition
> > > management a la PartitionMagic,
> >
> > Actually I've never seen PartitionMagic. Which partitioning systems
> > does
> > it support, and how does it manage to unify the GUI for them?
>
> Just intel partitions, actually. :-) But their interface seems general
> enough to me that it could be used for apple partitions (I'll send you
> some screenshots).

Thanks. The rest of the interface might be good, but the use of colors is
terrible. :-P

What I planned for the intel add-on is not that different: A tree view at
the left, a panel for viewing and changing the parameters of the selected
partition at the right and below a view visualizing the disk layout
graphically.

What I don't understand about the PM interface is, why they didn't join
the left and the right view.

[...]
> > > The idea is that the functions will first do some checking to make
> > > sure the operating can be performed. If so, they'll spawn a new
> > > thread to perform the task, and return a unique id for the job. The
> > > job then can send progress update messages containing the job id to
> > > the specified BMessenger, where they can then be passed on to the
> > > user somehow so they know what's going on.
> >
> > Mmh, I would rather see the asynchronous operation to be optional.
> > Synchronous processing should be possible at least. Instead of the
> > job ID,
> > I would vote for the ID of the partition under modification to be sent
> > with the notifications, plus an optional template message to be
> > passed to
> > the method. I don't know, but for some reason, I suspect, that there
> > will
> > be very few applications allowing for more than one partitioning
> > process
> > at a time, anyway. ;-)
>
> I was just planning on one: Drive Setup. :-) Seriously, if you had a
> way to lock portions of the drive being modified at any given time, I
> see no reason why you couldn't work on other parts of the drive too.

Locking has to be provided by the kernel (the disk_scanner module) anyway,
making sure that one thread at most manipulates a partition/disk.

> And certainly one could repartition a second drive while the first was
> busy resizing a 20GB bfs partition to 10GB. :-)

Okay, okay, hobby partitioners will be happy to have that feature. ;-)

[...]
> > Partition.h
> > -----------
> >
> > * Path(): The comment confuses me a bit. What does it actually
> > return? For
> > the device, I suspect, the path to the `raw' device. For partitions
> > the
> > `virtual' devices, i.e. `/dev/disk/.../x_y'? If we support arbitrarily
> > nested partitions, will the names then be `x[_y[_z[...]]]'?
>
> Yes, that was my intent.
>
> > The return value is not const, so the returned string is owned by the
> > caller?
>
> I completely forgot to check for appropriate use of const and
> non-const, sorry. I intended that one to be const.
>
> > I would prefer a BString* parameter instead (and perhaps also
> > a char* parameter version).
>
> That'd be fine with me, though I think a const version of what's there
> already would also be reasonable.

A const requires, that the string is an actual member variable (using
memory), while otherwise it could be constructed on the fly (from the
`raw' device path and the indices).

> > Finally, usually it is better not to declare virtuals const. Though,
> > in this case we know all derived classes.
>
> Well, in that case perhaps the BString* version would be better.

I actually meant the const of the method, not of its return value. But as
I wrote, there probably won't be any other subclasses and thus it doesn't
matter.

[...]
> > * CountDescendents() may be rather heavy weight
>
> This I don't really see. Wouldn't it just be a recursive set of
> CountChildren() calls, all of which just check the size of fChildren?

Er, well, I was thinking of huge hierarchies. ;-)
At least it is O(n) with n == number of partitions, while something like
Flags() is always O(1).

> > (as is PartitionWithID*(), BTW).
>
> This is a leftover from the first draft, and actually I'm not sure I
> see the point of it when there's
> BDiskDeviceRoster::GetPartitionWithID().

Mmh, maybe the approach isn't intuitive enough. The fundamental idea is,
that the atomic piece of information one can get is a BDiskDevice object
with a complete partition hierarchy. The BDiskDeviceRoster `checks out'
such an object and returns a pointer to the contained BPartition with the
respective ID. This is indeed heavy weight.

The BDiskDevice method on the other hand searches the BPartition with the
ID in question in the already present hierarchy. Compared with the former
one it is light weight.

[...]
> > * I miss something like `bool IsNode()' or `bool IsLeaf()' or
> > something like that (a better name!), so that one can ask whether the
> > partition is just a partition tree node or a real data partition.
>
> IsPartitionable() == IsNode()
> !IsPartitionable() == IsLeaf()

Mmh, I was thinking that IsPartitionable() returns whether it is possible
to partition the partition. E.g. I could decide to partition a RAM disk
formatted with a file system with some partitioning system. So the
respective BDiskDevice would have a FS, but would also be partitionable.
But I suspect, you mean to handle that differently: By first changing the
type of the partition (Initialize()) and create subpartitions thereafter.

> IsMountable() is there 1) to signal a partition with a supported
> filesystem and 2) to allow for signalling of a partitioning system that
> can also be mounted. I don't know if the latter case is even worth
> supporting.

Does something like this exist?

> Do you think it would just be better to instead have
> IsContainer() == !IsData(), or something to that degree?

If I understand you correctly IsContainer() == IsPartitionable(), so I'm
fine.

> > There're
> > IsMountable() and CountChildren(), but I think neither returns the
> > exact
> > info. A partition may simply be not mountable because of the missing
> > FS
> > add-on, which doesn't make it a tree node.
>
> In that case it would be !IsMountable() && !IsPartitionable()
>
> > And having no children
> > holds
> > true also for a tree node with all children removed.
>
> IsPartitionable() && CountChildren() == 0

Yes, IsPartitionable() seems to be what I was looking for. The name was
just suggesting something different to me. Maybe IsContainer() would be a
better name. Though it doesn't really matter, if it's documented properly.

[...]
> > * MoveTo(), Resize():
> >   - For performance reasons a MoveAndResize() could be added, too.
>
> Perhaps, though I think the BPartitionJob concept is better.

There would be another alternative: The BPartition methods could work
offline, and would be carried out, when a special (BPartition:: or
BDiskDevice::?)Commit() method is called. That way there isn't the problem
with hierarchies I see with the BPartitioningJob approach.

> >   - Who does the parameter checking? Most partitioning systems support
> >     block aligned partitions only, some may require even greater
> >     alignments (e.g. cylinder boundaries). Wouldn't that information
> >     be
> >     missing in a generic GUI?
> >     E.g. I imagine the user setting a new size for a partition,
> >     pressing
> >     `OK' and getting an error message like `Failed to resize
> >     partition:
> >     Invalid parameters.'
>
> You're right, there are a lot of assumptions built in there. I
> invisioned it as this:
>
> 1) Block alignment I kind of figured was a given, i.e. the GUI would
> ensure block alignment.

That's the very least. BTW, is it settable in PM? The second screen shot
you sent me has a `Cluster Size' field...

> 2) Size limits would be known from the GetSizeLimits() function.

For minimal and maximal size that assumption is valid, I would think.

> 3) Movement and creation limits could be inferred from the known
> partition layout. This I'm not so sure about now, though. Are there
> requirements wrt having a certain amount of space between partitions?

Not for the intel style (at least I don't know of any). But for other
systems that might apply.

> Thus the GUI would keep the user from making poor choices most of the
> time (i.e. unaligned, invalid size). The kernel code would do the
> final, complete error checking, but theoretically the GUI would have
> filtered out the majority of bad inputs.

If `most of the time' includes everything but user-caused problems -- like
creating a large file on a partition scheduled for shrinking to minimal
size -- that's fair enough.

> As to alignment on cylinder boundaries or what have you...isn't that a
> bit unlikely in this day and age?

The specs I read recommended to create partitions on cylinder boundaries,
for some systems require it to be like that. The Linux fdisk offers
cylinder alignment only (in expert mode this restriction might be omitted,
not sure).

> I thought you couldn't even believe
> the physical geometry claimed by hard disks anymore, as they oftentimes
> take the liberty of remapping blocks at the hardware level when they
> feel like it.

Mmh, I wouldn't know about that.

> >   - Complex operations are a must. The user must be able to specify a
> >     complete set of partitions layout changes, that are carried out
> >     together. I imagine something like a BPartitioningJob class with
> >     {Create,Resize,Move,...}Partition() methods and, of course, a
> >     Do()/Perform()/Execute()... (That makes MoveAndResize()
> >     superfluous,
> >     BTW.) This is more complicated, if one intends to allow creating
> >     hierarchies (e.g. creating a child of a not yet created
> >     partition).
>
> Well, if all the necessary parameters for partitioning could be
> retrieved from the appropriate add-ons (i.e. size limits, spacing
> between partitions, etc.), it could be done. The hard part is
> thoroughly gathering all the necessary information (which clearly the
> current design does not account for). Things like "only one extended
> partition is allowed" would have to be conveyed, also, and that could
> get difficult to do in a general manner, I think.

Maybe an approach (only) based on information gathering is not optimal. It
is virtually impossible to foresee every possible petty restriction a
partitioning system might have. Instead the add-ons could be asked
whether/what actions are possible in a certain situation. Like, when the
user selects a partition an add-on hook,
bool CanCreateSubpartition(BPartition*), could be invoked to find out
whether the `Create Partition' button must be disabled. For handling
partition location and size something like
`AlignPartition(BPartition*, off_t*, off_t*)' could be deployed to do a
live correction of the user's input.

[...]
> >   Furthermore, you return BDiskScannerParameterEditors. They are not
> >   exactly convenient to use. That's one reason why they didn't appear
> >   in
> >   the old API. The other is, that the class could be kept private, so
> >   that it could be changed without affecting the DiskDevice API.
>
> Okay. It seemed to me the old API was pretty much a "here, get all the
> necessary info and return when you're done" sort of thing, whereas I
> just wanted to get a BView that could be placed wherever needed.

Yes, that would work. We should thouroughly check that the interface is
fine, though.

> > * Initialize() misses a volume name and a flags parameter.
>
> I figured that would be incorporated into the parameters string, since
> initialize is also used for partitioning systems, which do not by
> necessity require a volume name. Shall I put them back, then?

Maybe you're proposal is better. In fact not even all FSs support volume
names. And for the other ones different length/character set restrictions
apply.

I compared BPartition with its former incarnation and found some more
points:

* Type(): What about the intel partition type vs. FS type issue? I
suspect, the former one is ignored?

* VolumeName(), FileSystemFlags() (perhaps not needed any more),
IsMounted(), GetVolume(), GetIcon(), Mount(), Unmount() are missing.

[...]
> > DiskDeviceRoster.h/DiskDeviceList.h
> > -----------------------------------
> >
> > * They look mostly unmodified, save regarding the disappearance of
> > BSession.
> >
> > BTW, have I ever mentioned BDiskDeviceList? I introduced it at some
> > point,
> > starting as kind of a prototype study, but I can't remember mailing
> > anything to the list about it.
>
> I don't know that you did. I remember being a bit surprised by it, but
> figured I must have just forgotten about it. :-) I take it that it's
> meant to be a repository of BDiskDevices that can subscribe for updates
> and thus remain up to date?

It can be used similar to the DeviceList class of the DeviceMap API --
i.e. you can get a complete list of devices that can be updated explicitly
by the user -- or it can be attached to a looper and keep updating itself.
In the latter case it works like the Tracker's AutoMounter (save the
auto-mounting, of course ;-). There are hooks called when the respective
notifications arrive, so one can create a subclass and listen to them this
way.

> > I'm not completely happy with it
> > anyway,
> > but that's another story...
>
> Oh do tell. :-) I'm actually not seeing what it is you wouldn't like
> just by looking at it, if that's any consolation.

:-)
As a static list it is OK, I think, but the updating on notification could
work better. I don't even think the problem is just the implementation of
the class itself, but also the way the notifications work.

If you re-partition a disk, then the registrar recognizes that some
partitions have been added and some removed and it sends a notification
message per recognized change. The BDiskDeviceList receives the first
message of the incoming series of notifications, updates the concerned
BDiskDevice and calls the hook method. Since updating the device really
brings the object up to date, it is already up to date, when the second
notification arrives. This is, what I don't like.

> So, in summary, clearly the API is lacking in the area of acquiring all
> the necessary information to allow it to be used as a basis for a
> general partitioning tool. As I mentioned, I thought it was worth
> looking into, and having done so, I'm starting to feel more like the
> previous system will be more cost-effective for us at this point, i.e.
> I still like the general concept, but it looks like it'll be a good
> deal more work than I had hoped, and for little gain over the old
> system. What do you think?

I think, we should at least investigate a bit further before dropping the
idea. If we really don't find a reasonable solution, we can still go with
the previous system.

CU, Ingo