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

[Tyler Dauwalder <tyler@xxxxxxxxxxxxx>]

> > > That reminds me: How would you want to treat empty partition table
> > > slots
> > > in the API=3F 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())=3F
> > 
> > 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=3F
> 
> While I agree that most users won't care, e.g. for Linux the physical
> indexing is of some importance -- if our DriveSetup `magically' 
> changes
> it, Linux might be rendered unbootable. Mmh, maybe we shouldn't care
> and assume that Linux users know what they are doing.

Well, I also figured we would do the right thing wrt indexing (it's 
physical ordering matching partition table index ordering you're 
referring to, right?), including reindexing if necessary.

> [...]
> > > 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.
> 
> Then the `Disk' column is rather superfluous, isn't it=3F

Hmmm, I guess so. Maybe it shows everything in both views, I don't 
remember. Either way, there's some redundancy in there, you're right.

> > Earlier versions lacked the left view.
> 
> So it has been `improved'. ;-)

Indeed. :-)

> [...]
> > > > > Partition.h
> > > > > -----------
> > > > >
> > > > > * Path(): The comment confuses me a bit. What does it actually
> > > > > return=3F For
> > > > > the device, I suspect, the path to the `raw' device. For
> > > > > partitions
> > > > > the
> > > > > `virtual' devices, i.e. `/dev/disk/.../x=5Fy'=3F If we support
> > > > > arbitrarily
> > > > > nested partitions, will the names then be 
> > > > > `x[=5Fy[=5Fz[...]]]'=3F
> > > >
> > > > Yes, that was my intent.
> > > >
> > > > > The return value is not const, so the returned string is owned
> > > > > by
> > > > > the
> > > > > caller=3F
> > > >
> > > > 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.
> 
> Maybe a GetPath(Path *path) makes most sense.

Okay. I like that.

> > > > > 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,
> 
> Since there are only two classes and user-defined subclasses doesn't
> make sense, you're right, it is.
> 
> > and I'm not sure I see
> > the reasoning why const virtuals should be avoided.
> 
> The reason is that by defining a virtual method of a base class const,
> things like lazy initialization or locking can't be used in derived
> classes. Well, there's `mutable' and as ultimate fallback casting, but
> that's not exactly nice.

Ah, I get it now. Thanks.

> > > At least it is O(n) with n =3D=3D 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.
> 
> Personally, I see little use in getting the number of all descendents,
> but if you like it. :-P

No, it's gone.

> 
> > > > > (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.
> 
> If you have other ideas, don't hesitate to tell -- it's not like I'm
> particularly fond of this approach.

I'm just not sure how useful the lightweight version really is. I can 
see having the general version, certainly, but the multiple versions 
seems a little like overkill.

> 
> [...]
> > > > > 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() =3D=3D 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() =3F SupportsPartitions() =3F
> 
> I just realized, that the naming is already a bit inconistent: We have
> PartitionAt() and PartitionWithID(), but CountChildren(). I think, I'd
> prefer ChildAt(), ChildWithID() CountChildren() and 
> SupportsChildren().
> Or most consequently, but a bit long (though it doesn't so look long 
> on
> a 15" 1400x1050 display ;-): ChildPartitionAt(),...

I think I like the Child names the best.

> BTW, how about a `BDiskDevice *BPartition::Device() const'=3F

Yes, good idea.

[...]
> > > 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=3F
> 
> Then the notification messages with the (more) precise event
> descriptions make little sense. A solution would be, if the internal
> update of a BDiskDevice/BSession/BPartition had invoked call backs
> corresponding to what had actually happened. But that wasn't how the
> framework was designed, so it would have been difficult to implement 
> it
> this way.

Well, unless the precise event messages also contain the necessary info 
to update a given set of objects without consulting anyone else, I'm 
not sure I see the usefulness in them over a general "things have 
changed" event.

-Tyler