[openbeosstorage] Re: The New Device API (tm)
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: openbeosstorage@xxxxxxxxxxxxx
- Date: Wed, 29 Jan 2003 01:38:38 +0100 CET
"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 :-
)
> > 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=5Ft, if it is. And if you have that, you also want
> notifications, when this information changes. So finally you have
> what
> is listed above...
Yeah, okay :-)
> 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.
And that doesn't come up with mean methods like LowLevelFormat() :-))
> What I'm thinking about is to drop BPartition::MountedAt() (and
> extended=5Fpartition=5Finfo::mounted=5Fat, BTW) and
> BPartition::GetMountPointNodeRef(). The user has VolumeID() to get a
> BVolume (or also a `status=5Ft GetVolume(Volume*)' could be added),
> which
> in turn can provide those data.
Yes, would make sense, and remove some of the overlapping :-)
> 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.)
Okay.
> > 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.
[...]
> 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 :-)
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 :-)
> 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).
> 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.
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 :)
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.
> > 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.
> ...
> > - 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
- that process takes some time, so it might be nice to have some
feedback while the formatting is in progress.
Anyway, *please* don't add this method to the unit tester :-)))
> > - 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.
> > 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.
> 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.
Okay.
> > 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.
> Tracker uses it. (We could also add a method returning the
> device=5Fgeometry::device=5Ftype. Maybe it's of interest.)
Why not=3F :-)
> > 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.
> > > * BPartition: Replaces Partition. Will provide similar
> > > functionality.
> > What does IsEmpty() mean here=3D3F
> 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.
Yes, that's probably a good reason; try to keep those IDs as persistent
as possible :-)
> 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()).
> 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 :)
> > 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
> > 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 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.
> > 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.
> > (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
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.
> > FileSystemFlags() is difficult - at least a file system would need
> > to
> > return a value without having a mounted file system - at least for
> > B=3D5FREAD=3D5FONLY 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=5FREAD=5FONLY 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.
Okay, then.
> > 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.
Adios...
Axel.
Other related posts:
