[openbeosstorage] Re: DiskDevice API v2.0
- From: Tyler Dauwalder <tyler@xxxxxxxxxxxxx>
- To: openbeosstorage@xxxxxxxxxxxxx
- Date: Sun, 30 Mar 2003 23:39:06 -0800
[BPartition as base class]
> > > 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.
Good deal, so that part at least can stay, correct? :-)
> > > 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())?
Actually, I think I was intending only to return non-empty partitions
in the C++ API, and then let the kernel level stuff do any magic
related to physical indexing (I believe this is how PartitionMagic does
it, actually). That seemed most appropriate for a user level type API.
What do you think?
> > 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.
Okay.
> > 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.
Okay by me.
>
> > > > 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
Very true. It's kinda nice for quickly seeing what partitioins are
formatted with what fs, but it's pretty ugly.
> 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.
Because then it wouldn't look like Windows Explorer :-P. That's my
guess at least. Plus the left view shows all disks if you have more
than one, while detailed info for the selected disk is on the right.
Earlier versions lacked the left 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.
Fair enough.
> > 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. ;-)
Exactly. :-) Seriously though, I once resized a (much too full) NTFS
partition and it took > 30 hours. It'd've been nice to have been able
to do other things meanwhile (that was also when PartitionMagic was a
DOS program and took over your entire computer).
> [...]
> > > 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).
Either way is okay by me. We should be consistent, though.
>
> > > 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.
Oh okay, I follow you now. As an aside, this seems like a perfectly
reasonable example of a useful const virtual, and I'm not sure I see
the reasoning why const virtuals should be avoided.
> [...]
> > > * 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. ;-)
We -are- a desktop OS, remember. ;-)
> At least it is O(n) with n == number of partitions, while something
> like
> Flags() is always O(1).
The hierarchy's still all in memory though... I guess it comes down to
whether that functionality is going to be needed enough to warrant the
function, or whether users should be forced to use the Visit()
functions if they want that info.
> > > (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.
Okay, I see. That does make sense once you know how it's intended to
work.
>
> [...]
> > > * 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.
No, I figured any partition is formattable with any system in general.
> 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.
Yes.
> > 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?
Not that I know of, but it could. :-)
> > 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.
Okay.
> > > 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.
SupportsChildPartitions() ? SupportsPartitions() ?
> [...]
> > > * 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.
Hmmm, that's actually what I was thinking you meant by
BPartitioningJob. :-) I.e., having an internal BPartitioningJob object
keeping track of the job sequence as the various changes were made. At
any rate, I think I like something like that best.
> > > - 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.
Yes, it does. :-)
> > 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).
Wow. Okay.
> > 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.
I don't really know either, but I thought I read something to that
degree in Dominic Giampaolo's book.
> > > - 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.
Okay, I think that sounds like a good idea.
> [...]
> > > 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.
Okay.
> > > * 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?
Well, it could (should :-P) be included if unrecognized, a la
"Unrecognized Partition (intel, 0xeb)" or whatever... ;-)
> * VolumeName(), FileSystemFlags() (perhaps not needed any more),
> IsMounted(), GetVolume(), GetIcon(), Mount(), Unmount() are missing.
Ooops. :-) FileSystemFlags() was intentional, the rest were overlooked
and should be added back.
>
> [...]
> > > 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.
Oh, I see. I thought the updates contained the necessary info for
updating manually, I guess. In this case then, only one hook is really
needed to know when to update then, correct?
> > 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.
All right, we should make up a list of functions needed for supporting
the general UI, then. I'm out of time for tonight, but will start on
one tomorrow if you don't beat me to it. ;-)
-Tyler
Other related posts:
