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

Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx> wrote:
[notifications]
> 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

Shouldn't there better be a notification that the partitions are gone=3F 
The reason why they are gone is okay to know, but not the other way 
around; knowing the reason and having to find out about the 
consequences yourself, right=3F
BTW, AFAIK, Tracker don't maintain a local list, but recreates the list 
everytime it's required.

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

Yes, that's right, but again, I think it would make more sense if 
Tracker would receive a "new volume in town" notification, not a "FS 
has been initialized" one.

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

Sure, that would be nice (and would speed up the mount menu in Tracker 
a bit :-))

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

Right; and we can change the internals anyway. Perhaps we shouldn't 
make the current APIs public now anyway, so that we don't have to 
support them later on, if we decide to drop/replace them.

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

Not at all, there are still more urgent issues, I think :-)

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

Right, although device IDs of mounted volume will be incremented with 
every new volume, so that's very unlikely. OTOH if you are using the 
path to get the device (or partition) directly, sure, then you would 
have that problem; of course you'll rarely do that anyway (and expect 
something in particular when you do it).

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

Yes, sorry, that makes sense.
BTW I think it would be nice if the header name for BDiskVisitor class 
would match with the class name (not BDiskIteration.h but 
BDiskVisitor.h).
That generally makes finding a particular header much easier.

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

Right - perhaps that's why it's better to use VisitEach() to 
differentiate the concept from DoForEach(); although we actually could 
have both (one with a dynamic visitor object, one with a static 
method).
What do you think=3F

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

Okay, if it can't fail, then I think it's okay to return true/false in 
the way you suggested it.

[...]
> Public is also the respective ioctl(), though one might consider it 
> less
> easily accessible. :-)

Right :-)

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

Hm, OTOH I also don't see how one can get feedback when you call the 
ioctl() directly. Perhaps you should ignore me and follow your main 
reason :-)

> > Anyway, *please* don't add this method to the unit tester :-)))
> *evil laughter* ;-)

Oh, please :-))

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

Oh, actually I don't think that we should drop that method. Finding out 
the name of a volume before it is mounted is a necessary feature.
I was referring to the wrong impression I had, because 
BDiskDevice::Name() returned the device path (which I didn't like).

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

We could have a unique ID for every disk device that's just incremented 
with every change; that would be an easy check if the changes are 
allowed or not.
But that number would also have to be incremented in the local object 
if that caused the change, of course :-)

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

Okay... although, that information is still available for GUI 
partitioning add-ons=3F

[...]
> > Hm, okay then we should probably drop VolumeID() completely - 
> > AFAICT it
> > returns the dev=3D5Ft for a mounted volume, right=3D3F So why not just 
> > have
> > this GetVolume(Volume *) method and only that=3D3F
> Besides the fact that a dev=5Ft 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.

I think you can; I think we can legally force someone to use the C++ Be 
API when he started to use it anyway :)
And then, a BVolume object should be fairly cheap.

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

That might also be a good idea, so that no one will try to use the old 
mount() version with the new API :)
Perhaps we should even drop the mount() name completely and replace it 
for safety=3F

> > Right, but what use do they have right now=3D3F I would just drop 
> > them -
> > the file system capabilities should be enough.
> OK.

BTW - if it turns out we have stripped down too many convenience 
functions, we can still and easily add them later; the other way around 
is the hard way :-)

> > 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=5Fdirectory() 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.

If it already asks for the file system name, it needs one kernel call 
to do that - if it would replace that with the call to get the directy 
from the file system directly, that wouldn't change much, I think.

> > 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=3D5F1 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=3F (Not completely lazy, if I
> understand your last sentence correctly.)

Yes, I would do it like this:
- don't do anything if not asked for
- if you've published an entry, take care that it stays valid (i.e. up-
to-date)

So it's lazy publishing and active maintaining of published entries :-
))
Note, normally, a mounted file system shouldn't be able to access its 
entry anymore if changes took place for that one, but then, that's 
exactly what we want in the future with online resizing and such :)

Adios...
   Axel.

Other related posts: