[openbeosstorage] Re: DiskDevice API v2.0
- From: Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx>
- To: openbeosstorage@xxxxxxxxxxxxx
- Date: Tue, 25 Mar 2003 23:50:51 +0100 (MET)
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(), Parent(), {Minimum,Maximum}Size() (well, they
could be computed, but make little sense), MoveTo(), ResizeTo().
On the other hand that would probably introduce more need for casting and
make the API less convenient to use.
[...]
> 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.
> 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.
> 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?
> 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.
> So, the aforementioned operations are all supported by the API now,
> along with partition initialization. I also made all such operations,
> many of which will probably tend to be rather lengthy processes, into
> functions of the form:
>
> int32 operation([args], BMessenger progressMessenger)
Good idea. We need to add support in the kernel for that, but that
shouldn't be a problem.
> 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. ;-)
Now to the headers:
DiskDeviceTypes.h:
-----------------
* Instead of macros, I would vote for string constants (`const char
*const').
* There is a way to discriminate between a CD and a DVD?
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[...]]]'?
The return value is not const, so the returned string is owned by the
caller? I would prefer a BString* parameter instead (and perhaps also a
char* parameter version).
Finally, usually it is better not to declare virtuals const. Though, in
this case we know all derived classes.
* Flags() doesn't harm, even, if we had convenience methods for each flag.
* `const BPartition* Parent()' -> `BPartition* Parent() const'?
* PartitionWithID() const?
* CountDescendents() may be rather heavy weight (as is
PartitionWithID*(), BTW). Is it really needed?
* 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(),...?
And hey, you left out the header of the BDiskDeviceVisitor class! :-P
It actually would only need one Visit() method now (with BPartition*
parameter).
* 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. 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. And having no children holds
true also for a tree node with all children removed.
* {Minimum,Maximum}Size(): Depending on how the values are obtained, a
single GetSizeLimits(off_t*, off_t*) may be better. BTW, why not const?
* MoveTo(), Resize():
- For performance reasons a MoveAndResize() could be added, too.
- 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.'
- 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).
* Regarding the parameter editors. It occurs to me, that you want them
for editing only partitioning system specific properties, but not
position and size. While that is a good idea in principle, it has
problems with specific restrictions applying to position and size (as
mentioned above).
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.
* Initialize() misses a volume name and a flags parameter.
* CreateChildPartition(): Regarding the position and size arguments the
same arguments as for Move() and Resize() apply.
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.
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'm not completely happy with it anyway,
but that's another story...
CU, Ingo
PS: I hope, I didn't miss to much. I'm quite tired. :-)
Other related posts:
