Re: DiskDevice API v2.0

Tyler Dauwalder <tyler-cyCl/[email protected]> wrote:
> 
> [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=3F :-)

Yep. :-)

[...]
> > 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.

[...]
> > 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

> Earlier versions lacked the left view.

So it has been `improved'. ;-)

> > > 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=5Fscanner module) 
> > anyway,
> > making sure that one thread at most manipulates a partition/disk.
> 
> Fair enough.

Perhaps it is a good idea to introduce BPartition::Lock()/Unlock() 
methods. The manipulating methods would require the partition to be 
locked. Locking a partition would also lock all its child partitions 
recursively. The locking was, of course, implemented in the kernel, so 
that it had global effect.

[...]
> > > 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.

Oh, that doesn't sound very efficient. :-)

> 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). 

I think, our implementation will be both, efficient and multitasking. :
-)

[...]
> > > > 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.

> > > > 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.

> > [...]
> > > > * 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=3F
> > 
> > Er, well, I was thinking of huge hierarchies. ;-)
> 
> We -are- a desktop OS, remember. ;-)

You don't know my secret plans yet. ;-)

> > 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

> > > > (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 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() =3D=3D IsNode()
> > > !IsPartitionable() =3D=3D 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.

That should be safe enough to assume.

[...]
> > > > 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(),...

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

> > [...]
> > > > * 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::=3F)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.

No, I was originally thinking of a separate class, while the 
BDiskDevice/BPartition hierarchy would have been immutable.

[...]
> > > 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.

I haven't read it yet. Er, I haven't even bought it yet. ;-)

[...]
> > > > * 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=3F
> > 
> > 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=3F I
> > suspect, the former one is ignored=3F
> 
> Well, it could (should :-P) be included if unrecognized, a la 
> "Unrecognized Partition (intel, 0xeb)" or whatever... ;-)

That should be good enough.

[...]
> > > > DiskDeviceRoster.h/DiskDeviceList.h
> > > > -----------------------------------
> > > >
> > > > * They look mostly unmodified, save regarding the disappearance 
> > > > of
> > > > BSession.
> > > >
> > > > BTW, have I ever mentioned BDiskDeviceList=3F 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=3F
> > 
> > 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=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.

> > > 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=3F
> > 
> > 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. ;-)

Very unlikely. :-P

CU, Ingo