[openbeosstorage] Re: The New Device API (tm)
- From: "Ingo Weinhold" <bonefish@xxxxxxxxxxxxxxx>
- To: openbeosstorage@xxxxxxxxxxxxx
- Date: Tue, 28 Jan 2003 03:36:25 CET (+0100)
> "Ingo Weinhold" <bonefish@xxxxxxxxxxxxxxx> wrote:
> > as promised I worked out a draft for a new Device API. Let me list
> > general requirements first:
>
> Nice!
> In fact, I would have let you decide if you wanted to do this - my
> opinion about the Device API is: either recreating it 100%
> compatible,
> or making a complete and nice replacement.
> But of course, I prefer the latter - I like to have clean APIs :)
So do I. :-)
My concern was the amount of time it would take to rewrite the API. But
I also start to tend towards doing it.
> > 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.
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.
> > Like the old API the new one shall only deal with disk devices (`/
> > dev
> > /
> > disk/...'); mounted file images are ignored.
>
> Well, if mounting/unmounting notifications are part of the class,
> then
> they are not completely ignored (nitpicking me) :)
That one I can counter. ;-)
The mount/unmount notifications one could get through the Device API
would only concern storage devices not images. (In particular the
notification message would contain a partition ID that can be used to
get a BPartition.)
> > The disk=5Fscanner as implemented seems to basically meet the
> > requirements. The notification support still needs to be added, of
> > course. Unless I'm mistaken, some notifications needed for the user
> > API
> > are not yet sended by the kernel, namely for low-level formatting,
>
> Where do you think those notifications are necessary/helpful=3F
The point is, that I want to make any polling in userland (at least for
the Device API users) superfluous. So anything that reshapes the layout
of a device (i.e. affects partitions) must trigger a notification. The
Tracker for instance wouldn't need to do anything but listen to the
notifications; the AutoMounter wouldn't be needed any longer.
> BTW for hot-pluggable devices, the whole disk could go away at any
> point in time.
Yep, that's what I intended B_DEVICE_REMOVED for. :-)
> > media changes and appearance/disappearance of devices. Unless there
> > are
> > reasons why this shouldn't be done, I would like to see them added
> > to
> > the OBOS kernel. For development and test under BeOS R5, at least
> > the
> > latter ones can be worked around by polling.
>
> Not only for R5 - the kernel have to poll for media changes as well,
> but of course, that's the right solution. The kernel should take care
> of that, and sending out notifications is a good thing.
Exactly my thought.
> > As already outlined in an earlier mail, I think, the best way do
> > deal
> > with the different change events is to bundle all the basic
> > activity
> > in
> > one entity. In principle that entity could live in the user
> > application, but since it will need to run at least one extra
> > thread
> > (a
> > BLooper; maybe another one for polling), it is probably better to
> > place
> > it in a server, namely the registrar. The additional communication
> > overhead should be acceptable, even if each method call would
> > require
> > a
> > message exchange with the registrar (similar to the BMimeType
> > implementation). If iteration turned out to be slow, optimizations
> > were
> > possible.
>
> Dunno, we could also just copy the current node monitor behaviour -
> the
> kernel would then send out the notification directly to the correct
> receiver.
> It would be pretty easy to "abuse" that mechanism for these things -
> in
> fact, it's already planned to do so for mount/unmount events.
The reason, why I want to bundle all activity in the registrar instead
of letting the kernel send the messages directly to the applications is
twofold: 1) The registrar has information that kernel doesn't have,
e.g. the partition ID I intend to send with each message concerning a
partition. 2) I'd really love to play with another way to deliver
notification messages -- like recently brought up on the kernel list,
to use shared memory to pass the event notifications to the registrar
which in turn delivers the messages. Since the registrar is involved
anyway, it is quite a good opportunity to do that.
> > Before continuing, I should introduce the classes I envision for
> > the
> > API:
> > * BDiskDeviceRoster: The roster that knows about disk devices. It
> > can
> > be used to iterate through the devices and to register/unregister
> > for
> > notifications
>
> I don't get the ID part - what exactly is that, and how can it be
> persistent=3F
The partition IDs do already exist in the old Device API. They identify
partitions uniquely in a given DeviceList. Since there would be one
global list in the registrar, the IDs are fixed through a session (till
shutting down the system, I mean). They are of course not persistent,
i.e. even, if you eject a CD and re-insert it, the partitions on it
will be mapped to different IDs, but that doesn't matter. The only
point is to identify a partition temporarily, and I don't see any other
way to do that. The path to the partition device for instance
identifies another partition, when you eject a CD and insert another
one.
The IDs for sessions and devices are my idea. I was struggling a while,
but in the end I decided to find it consequent to have them.
BTW, I'm thinking about changing the following iteration methods:
bool EachDevice(BDiskDeviceVisitor *visitor, BDiskDevice *device =
NULL);
bool Each[XYZ]Partition(BDiskDeviceVisitor *visitor, BDiskDevice *
device = NULL, BPartition **partition = NULL);
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).
> 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! ;-)
> Also, a "what" field for the message is not defined yet.
>
> > * BDiskDevice: Replaces Device. Will provide similar functionality.
>
> I would suggest the following name changes (again, I am not a native
> speaker, I would just prefer them for whatever reason, feel free to
> ignore me :-))
> - NoMedia() to HasMedia() or similar
OK.
> - Format() to LowLevelFormat() (would at least save a comment ;)
Yep.
> - Name() to Path() or DevicePath()
Alright.
> - GetDisplayName() to GetName() or something similar, maybe also have
> a
> "char *" version of it to follow the (in this regard) broken Be API=
> 3F
Can be done.
> - again "ForEach...()"
Right.
> What exactly does Traverse() do=3F Iterate over all sessions/
> partitions=3F
Yes, it traverses the tree (pre-order). One can for instance write a
little visitor to output the structure. I just couldn't resist to
implement that design pattern here. :-P
> 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.
> 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.)
> 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...
> > * BSession: Replaces Session. Will provice similar functionality
> > (partitioning will be new).
>
> Index() is the index in the list of sessions for this device=3F
Yep.
> IsVirtual() means there is no on-disk session structure, i.e. for
> hard
> disks=3F
Yep.
> > * BPartition: Replaces Partition. Will provide similar
> > functionality.
>
> What does IsEmpty() mean here=3F
For the DOS/Intel/... partitioning system, it corresponds to a primary
partition that is marked empty. One might asked, why those `partitions'
aren't just omitted. Good question, but they currently aren't -- maybe
to not break the indexing.
> 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.
BSession may return the partitioning style though. But there I would
prefer a string, too. One might add a second method that tries to
interpret that string, returning something like
B_INTEL_PARTITION_STYLE... or a `bool IsIntelPartitionStyle()'
> Perhaps rename VolumeID() to Device()=3F
Better not. That would be confusing (see `BDiskDevice *
BPartition::Device() const'). Since the dev_t returned by VolumeID()
can be used to construct a BVolume, it is quite well named. The only
misnamed thing is the dev_t, which should better be volume_t.
> For mount(): I want to have real parameter strings, so that you could
> replace "void *params" with "const char *params" as it is done in
> Initialize().
Is fine with me in principle, but would Mount() call the <unistd.h>
mount() with mount(..., params, strlen(params) + 1) then?
> I would also rename GetMountPointNodeRef() to GetMountPoint() and add
> versions of it to return a BEntry and a entry=5Fref.
Can be done.
> 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.
> (and I want to have a FindDirectory()-like call in the FS API as
> well)
Something with the same functionality as find_directory()? Shouldn't
that better be a BVolume method? Especially because the C++ only
version has a BVolume parameter anyway.
> 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?
> > 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?
> Anyway, have to go now, perhaps I'll add something later:
> [.......]
Thanks a lot for the comments! More input is of course always welcome.
CU, Ingo
Other related posts:
