[openbeosstorage] Re: The New Device API (tm)
- From: Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx>
- To: openbeosstorage@xxxxxxxxxxxxx
- Date: Wed, 29 Jan 2003 19:29:07 +0100 (MET)
On Wed, 29 Jan 2003, Axel =?iso-8859-1?q?D=F6rfler ?= wrote:
> "Ingo Weinhold" <bonefish@xxxxxxxxxxxxxxx> wrote:
> > My concern was the amount of time it would take to rewrite the API.
> > But
> > I also start to tend towards doing it.
>
> Now that you've already begun to design the class, it's hard to stop :-
> )
Exactly. :-)
> > 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.
>
> Actually, I like the differentiation between both classes; you will
> rarely need the Device API, but you'll often use the simple BVolume
> class.
Yep, you're right.
> And that doesn't come up with mean methods like LowLevelFormat() :-))
Hehe. :-)
> > > Where do you think those notifications are necessary/helpful=3D3F
> > 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.
>
> I am not sure about this; of course, there are many useful things (like
> media changed) - but why would I want to get a notification if a disk
> has been formatted=3F In what way does this change help me.
In case of low-level formatting this means, that all partitions on that
disk have ceased to exist. If you maintain a local list like the Tracker
does, this is definitely of interest. FS initialization is for instance
interesting, if that changes the file system of the partition. E.g. the
Tracker might want to mount a new BFS partition.
I really want to get rid of any need for polling. So you can retrieve the
data once and then only need to listen for notifications.
> [...]
> > 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.
>
> To 1): that information could be easily shared with the kernel, of
> course :-)
Right. In fact, it might turn out, that we need to move everything I
intended to put into the registrar into the kernel to be able to implement
some of the fancier features you have in mind for R2+. It doesn't look
like that to me at this time, but the future concepts are currently a bit
vague to be sure. Er, but that's another topic... :-P
> To 2): sure, that's a reason, but I am not sure if we should play now
> with this - we could a) see it's slower as the kernel version or has
> other kinds of disadvantages, or b) it works really nice, and we want
> to keep it.
> Altough b) looks good at first sight, we would create an inhomogeneous
> and two-fold solution in the kernel.
> Of course, we could then also move the node monitor messages through
> that interface, but that would cause wasted resources on developing the
> former solution.
> I guess, I should just postpone the kernel implementation for
> messaging, and let you play around, so that we will get the right
> solution at first shot :-)
Mmh, if you're not in a hurry...
> > The partition IDs do already exist in the old Device API. They
> > identify
> > ...
> > 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.
>
> I would be in favour for IDs for sessions, but I'd say that the device
> ID is not that nice; as there is always a real device directly
> accessible anyway - having a separated ID makes things more
> complicated, I think (not sure though).
I'm not sure either, but actually the situation is the same as with
partitions: hot pluggable devices can be changed by the user, and if you
only have the device path, you may speak to another device the next time
you are using it. That's perhaps quite unlikely -- especially considering
that respective notifications will be sent -- but at least possible in
principle.
> > bool EachDevice(BDiskDeviceVisitor *visitor, BDiskDevice *device =3D
> > NULL);
> > bool Each[XYZ]Partition(BDiskDeviceVisitor *visitor, BDiskDevice *
> > device =3D NULL, BPartition **partition =3D 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).
>
> I don't see how those can achieve that, but I would have suggested you
> to extend the Visitor methods to also contain information about the
> Device and Session when you are iterating over the list of partitions.
If you have a BPartition object you can get its BSession and BDiskDevice
at any time (BPartition::Session()/Device()). Mind you that BPartitions
and BSessions won't exist without a parent BDiskDevice -- at least that's
how I imagine it.
> BTW I am not that familiar with the Visitor idea, but it looks good -
> those design patterns are rarely used in the Be API yet :)
Yep, in some parts I miss them quite a bit, especially the Interface Kit
could greatly benefit from using them. We could borrow a lot of good
ideas from Java for instance.
> Like Tyler, I am also not sure about the return code for the visitors -
> perhaps we should have a status=5Ft code which could also reflect
> problems when trying to iterate through the devices=3F Like media change,
> no memory, device does not respond, whatever.
Mmh, the return value wasn't intended to indicate an error. The
BDiskDevice methods can't even fail, and for the BDiskDeviceRoster
versions errors can gracefully be ignored -- I thought of iterating
through the BDiskDevices and let their methods do the job, so that in fact
only getting a BDiskDevice can fail.
> > > 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.
>
> Then I would say DoForEach() for consistency - although I like Tyler's
> suggestion VisitEach; it does better match the BDiskDeviceVisitor
> class.
OK, I will simply make a spontaneous decision between the two before
committing the headers. :-)
> > ...
> > > - Format() to LowLevelFormat() (would at least save a comment ;)
> > Yep.
>
> BTW perhaps we should drop that functionality altogether:
> - this feature is (and should be) *very* rarely used; I guess we'll
> need it exactly once in the whole OS, making it not a good candidate
> for a public and easily accessible API
Public is also the respective ioctl(), though one might consider it less
easily accessible. :-)
> - that process takes some time, so it might be nice to have some
> feedback while the formatting is in progress.
That's true. And I suspect, there is no way to get that feedback.
The main reason, why I added it, was for sake of completeness.
> Anyway, *please* don't add this method to the unit tester :-)))
*evil laughter* ;-)
> > > - GetDisplayName() to GetName() or something similar, maybe also
> > > have
> > > a
> > > "char *" version of it to follow the (in this regard) broken Be API
> > > =3D
> > > 3F
> > Can be done.
>
> Note that BVolume also has a GetName() method.
But that's of course the volume name. Or were you referring to the fact,
that we could also drop BPartition::VolumeName()?
> > > What exactly does Traverse() do=3D3F Iterate over all sessions/
> > > partitions=3D3F
> > 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
>
> Yes, nice. But I still think it would be nice if the partition visitor
> also had access to its parent session/device, then.
As mentioned before it can, using the Session() and Device() methods. :-)
> > > What exactly is IsFloppy() for=3D3F
> > It returns whether the device is a floppy drive. Is in the old API
> > and
>
> It's only used in things that should better be hidden from the user
> anyway (finding the right display name, or setting partition ID to -1
> because floppies don't have a partition - do we support that right now=3F
> :).
> We might let it in, but we should make sure there aren't any real
> reasons left that would require it for the user.
Yes, that's what I had in mind anyway.
> > > What exactly does Synchronize() and why is it necessary=3D3F
> > 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*)=3F). I'm not sure...
>
> Good question. I'd say Synchronize() or Update() is a good thing, then.
> OTOH all destructive methods should only work on up-to-date objects, or
> else things might get hairy.
Agreed.
> > 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.
>
> Hehe :-)
> I think it's only useful for file system add-ons, it doesn't have to be
> part of this API at all (PartitionCode()).
OK, will drop it.
> > 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=5FINTEL=5FPARTITION=5FSTYLE... or a `bool IsIntelPartitionStyle()'
>
> That might be okay, too. Or just have
> B=5FINTEL=5FPARTITION "partition/intel/v1" (or whatever) :-)
>
> Something easy to work with :)
I will see what I can do. :-)
> > > Perhaps rename VolumeID() to Device()=3D3F
> > Better not. That would be confusing (see `BDiskDevice *
> > BPartition::Device() const'). Since the dev=5Ft returned by VolumeID()
> > can be used to construct a BVolume, it is quite well named. The only
> > misnamed thing is the dev=5Ft, which should better be volume=5Ft.
>
> Hm, okay then we should probably drop VolumeID() completely - AFAICT it
> returns the dev=5Ft for a mounted volume, right=3F So why not just have
> this GetVolume(Volume *) method and only that=3F
Besides the fact that a dev_t is a bit more lightweight and can e.g.
directly be added to a BMessage, there's no real reason to keep
VolumeID(). So I can remove it.
> > > 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=3F
>
> I might break source compatibility there, and change it in the header
> to a "char *" as well, and remove the size.
> mount() is normally not used by applications anyway (furthermore, every
> file system tend to have its own mount command now [which I want to
> make superfluous).
I see.
> > > I would also rename GetMountPointNodeRef() to GetMountPoint() and
> > > add
> > > versions of it to return a BEntry and a entry=3D5Fref.
> > Can be done.
>
> Or drop them, as you suggested.
Will do.
> > > 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.
>
> Right, but what use do they have right now=3F I would just drop them -
> the file system capabilities should be enough.
OK.
> > > (and I want to have a FindDirectory()-like call in the FS API as
> > > well)
> > Something with the same functionality as find=5Fdirectory()=3F Shouldn't
> > that better be a BVolume method=3F Especially because the C++ only
> > version has a BVolume parameter anyway.
>
> You misunderstood me: I meant in the file system add-on API. And that
> should be accessible to the user via BVolume, correct.
> That way you could ask for the trash directory for a volume, for
> example.
> Do you think that makes sense=3F
Absolutely!
> Normally, the system defines the location of the trash, so it might be
> stupid to make this at the file system level - but it would ease the
> process of adding and integrating new writable file systems.
Currently the implementation of find_directory() does check the name of
the file system to get the respective location. That's pretty stupid and
unflexible. The only drawback I see, is that the function will become
more expensive when needing to ask the FS add-on.
> > > 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=3F
>
> Right, what I am thinking of is an on-demand service - the partitions/
> sessions are only scanned for two reasons:
> 1) you display the directory contents of /dev/disk/..../master/0/
> 2) you try to open /dev/disk/..../0=5F1 directly
>
> Of course, the kernel only updates this information if it changes them
> itself or if the media is changed.
I'm not sure, if I understand, what you mean. Would that be a kind of lazy
publishing, i.e. when you list the directory the kernel updates the
entries and you'll get an up to date list? (Not completely lazy, if I
understand your last sentence correctly.)
CU, Ingo
Other related posts:
