[openbeosstorage] Re: DiskDevice API v2.0
- From: Tyler Dauwalder <tyler@xxxxxxxxxxxxx>
- To: openbeosstorage@xxxxxxxxxxxxx
- Date: Thu, 27 Mar 2003 23:47:14 -0800
On 2003-03-25 at 14:50:51 [-0800], Ingo Weinhold wrote:
>
> On Sun, 23 Mar 2003, Tyler Dauwalder wrote:
>
> > Here's my proposed header changes for the Disk Device API
> > incorporating a more general partitioning scheme:
> >
> > http://www.dauwalder.net/DiskDeviceAPI_v2.0.zip
> >
> > BPartition now provides the base functionality used for any
> > contiguous chunk of data. Each BPartition has a char* Type() that
> > identifies the system it's formatted with (be it a partitioning
> > system or a file system).
> >
> > BDiskDevice is a subclass of BPartition that provides the device
> > specific functionality. Each device also has an associated char*
> > DeviceType() that identifies what sort of device it is.
>
> 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.
> 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.
> [...]
> > 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?".
:-)
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.
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.
> > Since the whole system is now
> > more general, it seemed to me it would be reasonable to support the
> > general set of partition management functions (move, resize, create,
> > delete, edit parameters) in the API itself.
>
> That may be more difficult than it looks. I write a bit more on that
> below.
Yes, I'm beginning to see what you mean. :-)
> > 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).
> > and I think supporting partitioning
> > in the API itself will make it easier for new partition systems to
> > be
> > supported transparently.
>
> In theory that is certainly right. In practice, however, I suspect,
> that
> the only application using the provided write support will be
> DriveSetup.
> And currently it does so in a very specific manner -- it doesn't
> manipulate single partitions, but the partition layout of a whole
> disk.
See, room for improvement. ;-)
> > 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.
And certainly one could repartition a second drive while the first was
busy resizing a 20GB bfs partition to 10GB. :-)
> Now to the headers:
>
> DiskDeviceTypes.h:
> -----------------
>
> * Instead of macros, I would vote for string constants (`const char
> *const').
Fine by me.
> * There is a way to discriminate between a CD and a DVD?
Not that I know of. :-) I had "Optical Storage Device" in there at some
point, but I'm really not sure what happened to it...
> 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.
> 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.
> * Flags() doesn't harm, even, if we had convenience methods for each
> flag.
Okay.
> * `const BPartition* Parent()' -> `BPartition* Parent() const'?
Yes.
> * PartitionWithID() const?
Yes.
> * 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?
> (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().
> * Traverse() could now also return a BPartition*. Another name for the
> method would be great (yes, I proposed it, but I never claimed it to
> be a
> good one ;-). Maybe VisitEachDescendent(), VisitPartitionTree(),...?
Well, if we keep CountDescendents(), VisitEachDescendent() would be
consistent at least. I don't have any better ideas atm :-).
> And hey, you left out the header of the BDiskDeviceVisitor class! :-P
Ooops. :-)
> It actually would only need one Visit() method now (with BPartition*
> parameter).
True.
> * 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()
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. Do you think it would just be better to instead have
IsContainer() == !IsData(), or something to that degree?
> 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
> * {Minimum,Maximum}Size(): Depending on how the values are obtained, a
> single GetSizeLimits(off_t*, off_t*) may be better.
Yes, I like that better.
> BTW, why not const?
Same story, no good reason.
> * MoveTo(), Resize():
> - For performance reasons a MoveAndResize() could be added, too.
Perhaps, though I think the BPartitionJob concept is better.
> - 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.
2) Size limits would be known from the GetSizeLimits() function.
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?
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.
As to alignment on cylinder boundaries or what have you...isn't that a
bit unlikely in this day and age? 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.
> - 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.
> * Regarding the parameter editors. It occurs to me, that you want them
> for editing only partitioning system specific properties, but not
> position and size.
Yes.
> While that is a good idea in principle, it has
> problems with specific restrictions applying to position and size
> (as
> mentioned above).
Yes.
> 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.
> * 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?
> DiskDevice.h
> ------------
>
> * Unset(): It was possible (and necessary) to create an uninitialized
> BDiskDevice object that BDiskDeviceRoster method could fill with life.
> Unset() simply resets the object to the uninitialized state,
> similarly to
> almost all of the SK classes. Whether it will be needed depends on
> the way
> we'll communicate with our kernel component.
Okay.
> 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?
> 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.
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?
-Tyler
Other related posts:
