[openbeosstorage] Re: The New Device API (tm)

> Now that things have a name, let's come to the main design decision. 
> Hrrmmmhh. (*clearing the throat* ;-) The question is, how static the 
> data shall be. Be's implementation is maximally static: You have a 
> DeviceList and as long as you don't call an update method explicitly 
> the data remain unchanged. The entirely opposite strategy is to always 
> return up to date information, i.e. two successive calls to a 
> BPartition's GetVolumeName() could return different results.
> 
> Personally I find the latter approach inconvenient. One had to always 
> check method results, since any method could fail at any time. Thus the 
> error handling would certainly be rather painful. So I tend to favor a 
> static strategy: A BDiskDevice and all its children should remain 
> unmodified, unless explicitly updated. That means, one can use 
> BDiskDeviceRoster to traverse a dynamic device list (GetNextDevice(), 
> Rewind()), but a BDiskDevice retrieved that way would be static. 
> BDiskDeviceRoster may of course also provide a method to get a snapshot 
> of the current list...

I didn't like this at first, but considering one can use the Device API
objects to get an initial snapshot and then maintain it via notifications,
I can see how this would make life easier for everyone concerned. In
other words, sounds good to me. :-)

> > > user API:
> > > 
> > > * convenient traversal of devices, sessions and partitions
> > > * advanced iteration (as provided by the old API)
> > > * FS initialization, partitioning, low-level formatting
> > > * ejection of removable media
> > > * notifications on changes, including the following events:
> > >   - mount point changes (renaming, moving)
> > >   - mounting/unmounting
> > >   - FS initialization, partitioning and low-level formatting
> > >   - media changes
> > >   - appearance/disappearance of devices
> > 
> > I am not sure if I like that - but the only reason for this is the 
> > relatively big amount of overlaps it has with BVolume & BVolumeRoster 
> > -
> >  I think it's better to have a clear differentiation between both 
> > classes.
> 
> Nah, it isn't that big. Basically only the mount/unmount events and the 
> data about mounted partitions. The rest is disjoint as far as I see. 
> But yes, you're right, there is overlapping, and I don't like it that 
> much either. But it is simply too obvious to also get information about 
> the mount state of a partition; even if it was only whether or not it 
> is mounted and a dev_t, if it is. And if you have that, you also want 
> notifications, when this information changes. So finally you have what 
> is listed above...
> 
> In fact, it could work quite well to completely drop BVolume[Roster] 
> and incorporate their functionality into the Device API. But we are of 
> course not free to do that.

Technically speaking, we *are* free to do that, aren't we? Certainly,
BVolume must be kept around for compatibility, but it could be 
deprecated and superseded by the Device API. The only immediate
reason I can see for needing to keep BVolume around (vs. moving
its functionality into the Device API) is if a BVolume object can 
be created for a mounted file image. But I'm afraid I really don't
understand the low level differences between the two (i.e. at
which point the system differentiates between a mounted device
and a mounted file image). Could someone explain that for me?
That aside, do you see other reasons for keeping BVolume[Roster]
that I don't?

> What I'm thinking about is to drop BPartition::MountedAt() (and 
> extended_partition_info::mounted_at, BTW) and 
> BPartition::GetMountPointNodeRef(). The user has VolumeID() to get a 
> BVolume (or also a `status_t GetVolume(Volume*)' could be added), which 
> in turn can provide those data.

That seems reasonable, too, though.

> bool EachDevice(BDiskDeviceVisitor *visitor, BDiskDevice *device = 
> NULL);
> bool Each[XYZ]Partition(BDiskDeviceVisitor *visitor, BDiskDevice *
> device = NULL, BPartition **partition = NULL);

Are you meaning these two+ functions specifically, or all the EachXXX 
functions? I think having a consistent interface all around for the
various EachXXX() functions might be rather nice.

> They would return `true', if terminated early, and in that case set the 
> return parameters (if supplied) accordingly. I believe, that if one is 
> interested in the concerned partition, then one usually wants the 
> object to get more info (moreover this can be done with zero overhead).

This bugs me: why does the function return an affirmative when
it fails and a negative when it succeeds? The only reason I can
see is to match the error checking sematics of the other versions
that return pointers, thus returning NULL on success. But if we
standardize the interface, wouldn't it make sense to use the
more obvious bool values for success and failure?

> > And I would like "ForEach" instead of "Each" better,
> 
> Yep, or even DoForEach* as in BList. I copied the naming from the old 
> Device API.
> 
> > but I am not a native speaker.
> 
> Me neither. :-)
> Tylaahhhhh! ;-)

Sorry, I'm here now. :-) I like "VisitEachXYZ()" best, but "DoForEachXYZ()"
seems reasonable, too. 

> > Should the Session class functionality be mixed with the BDiskDevice=
> > 3F 
> > (CountPartitions(), EachPartition()=3F)
> > I am not sure about this, but if 
> > it would be really convenient...
> 
> I think, it is quite convenient to have EachPartition() in BDiskDevice, 
> for CountPartitions() I'm not sure. But at least, I see no real problem 
> or potential for confusion -- the BSession versions concern the 
> partitions on that session only, while the BDiskDevice versions concern 
> all partitions on the device.

I think having said functions in both classes is a good idea.

> > What exactly is IsFloppy() for=3F
> 
> It returns whether the device is a floppy drive. Is in the old API and 
> Tracker uses it. (We could also add a method returning the 
> device_geometry::device_type. Maybe it's of interest.)

That might be more useful in the long run.

> > What exactly does Synchronize() and why is it necessary=3F
> 
> Since once got a BDiskDevice object is static, that method could be 
> used to update the object according to the current state of the device. 
> Well, it may be consequent to drop that method completely and declare 
> BDiskDevice, BSession and BPartition immutable; i.e. also Eject(), 
> LowLevelFormat(), Partition(), Initialize() would not affect the object 
> and the user would need to retrieve a new one to see the effects (or 
> have a BDiskDeviceRoster::Update(BDiskDevice*)?). I'm not sure...
> 
> > Should Type() be a string=3F Wouldn't it be better if we had IDs, 
> > like 
> > B=5FINTEL=5FPARTITION =3D 'intl'=3F But I might be wrong here :-)
> 
> For Intel style partitioning Type() is the interpreted PartitionCode() 
> (which I personally don't like, but nobody listens to me ;-), for 
> partitioning systems using strings it's the respective string.

How about a standardized encoding of the PartitionCode() value in the 
Type() string for unrecognized codes, i.e. ("Unrecognized Type 0xfe")? 
That would discourage use of the partition code over the type string
unless absolutely necessary, but still make it available. To tell you 
the truth, I'd like this anyway as a user. Seeing "Unrecognized Type" 
in DriveSetup or whatever is a lot less illuminating that "Unrecognized 
Type 0xfe".

> > I would also like to get rid of the IsBFS(), IsHFS(), IsOFS() methods 
> > -
> >  those are strange convenience functions to me.
> 
> Well, yes, they are. The only alternative would be to test 
> FileSystemShortName() for "bfs",... though.

I can see why IsBFS() might be oft used, but are IsHFS() and IsOFS() 
really needed anymore? And really, shouldn't the checks for attribute
or index support be used rather than just seeing if the fs is bfs?
That being true, I wouldn't see any good reason to keep any of those
functions.

> > FileSystemFlags() is difficult - at least a file system would need to 
> > return a value without having a mounted file system - at least for 
> > B=5FREAD=5FONLY this not possible; all other things should be simple 
> > (although not every BFS disk has to have indices).
> 
> It is completely sufficient to return B_READ_ONLY only, if the FS add-
> on doesn't have write support (the Device API can additionally check, 
> whether the device is read-only). The flags I'm mainly interested in, 
> are the ones still to be introduces, indicating support for 
> intialization and resizing.
> 
> > PublishDevice() should not be necessary - at least I would like to 
> > get 
> > rid of it, and let the kernel automatically create them as needed. 
> > Ideas welcome, I haven't thought much about it, I just don't like the 
> > way it's currently done :-)
> 
> The most simple way is to always publish all (non-empty, non-hidden) 
> partitions, but that's probably not what you are looking for?

Is there a good reason *not* to always publish all of them? The way
it's currently done has always seemed like an odd way of doing things.

If PublishDevice() *is* kept, shouldn't an UnpublishDevice() be available?

> > > Regarding the naming: I find BDevice simply too general. BDisk 
> > > would 
> > > be 
> > > fine in principle, but there may be potential for mixing it up with 
> > > BVolume. BStorageDevice would be most accurate, but also a bit 
> > > longer. 
> > > :-P
> > 
> > BDiskDevice is fine, I would like BStorageDevice better - although 
> > BDiskDevice better covers the topic, I think (dunno why, though).
> 
> I personally don't mind. Tyler? Lurkers?

/dev/disk => BDiskDevice makes sense to me. BStorageDevice is more
accurately general, yes, but considering we've already got ourselves
a disk_scanner API, I think sticking with BDiskDevice would be the 
better choice. Unless we feel like changing to storage_scanner... 
:-P :-)

-Tyler

Other related posts: