[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

That's a good point. I will change the notification events to something 
like B_DEVICE_PARTITION_{ADDED,REMOVED} then.

> BTW, AFAIK, Tracker don't maintain a local list, but recreates the 
> list 
> everytime it's required.

That does perhaps depend a bit on the point of view. The AutoMounter 
"AutomountScan" thread (AutoMounter::WatchVolumes()) periodically 
checks AutoMounter::fList (DeviceList) for changes and updates it if 
necessary. Nevertheless on some occasions AutoMounter::RescanDevices() 
is invoked (maybe sometimes even unnecessarily), which completely 
rebuilds the list.

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

OK, then I'd propose something like B_DEVICE_PARTITION_CHANGED for that 
case.

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

No, the API should definitely not be public in R1. I guess, now I also 
remember, why I put the headers for the session/partition/fs module API 
in the `private' subtree. :-) Since I don't think, they should be 
public in the first release -- but it seems that `headers/os/drivers' 
is the correct place anyway.

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

Hehe. And skiing of course. ;-)

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

Yes, that's right. However, the device and session IDs are not very 
prominent in the API anyway -- there's basically only the 
BDiskDeviceRoster::*WithID() method to get an object for an ID, and the 
IDs sent with the notification messages. Therefore I'd like to keep 
them for the time being and reconsider dropping them, when we've got 
some more experience with the implementation.

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

I planned that for the first check in anyway. :-)
When I started writing the file I had some different ideas I finally 
throw away leaving only the BDiskDeviceVisitor class.

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

Yep, VisitEach*() will make it, I guess. For the other iteration 
strategy. Mmh, I actually don't like it -- a callback function plus a 
void pointer as params is so non-OO...
As a compromise one may offer a visitor subclass that takes a callback 
function and params on construction, thus functioning as a wrapper.

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

That would require some support by the kernel and the concerned driver. 
For a function used that rarely, it's perhaps a bit of overkill.

> Perhaps you should ignore me and follow your main 
> reason :-)

Mmh, maybe I just do, what I can do best, and simply postpone the 
decision. ;-)

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

Ah, yes. And that will be changed of course.

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

I had something similar in mind, but I would make it a pure change 
counter and not mix it with the ID concept.

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

Since the GUI add-on will get the parameter string retrieved from the 
kernel module, it will have all the information it needs.

BTW, I already started to implement the partitioning support in the 
intel partition module. The actual writing of the partition map to disk 
is still missing, but that should be relatively simple.

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

Mmh, isn't mount() part of some POSIX standard? What we could do, is to 
keep it, but let it check whether the supplied void* parameter 
(considering len too) looks like a string, and, if it does, pass it to 
the new API.

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

You're absolutely right. It currently gets a fs_info from the kernel, 
so it wouldn't make any difference performance-wise.

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

OK, that's how I understood it.

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

Yeah! :-)

Regarding the further proceeding: I will have some time on the weekend 
and I plan to check in the proposed headers (incorporate the discussed 
changes before, of course), doxygen-document the API -- due to the non-
fixedness of the API maybe not too exhaustive -- and start with the 
implementation.

Tyler, I have the impression, that you're currently pretty real-life-
busy, so I'll proceed relatively `unteamish'. If my impression is 
wrong, just yell -- there's enough work and it shouldn't be hard to 
divide it more or less sensibly. :-P

CU, Ingo

Other related posts: