Opened 6 years ago

Closed 5 years ago

#10191 closed task (fixed)

Make NetFS stack an actual HPKG via build system

Reported by: mmadia Owned by: bonefish
Priority: normal Milestone: R1
Component: Build System Version: R1/Development
Keywords: Cc:
Blocked By: #10192, #11168 Blocking:
Has a Patch: yes Platform: All

Description

By making NetFS an actual HPKG, it could be included in the default image as a non-activated package. This would make testing easier for end users.

Attachments (2)

0001-Converting-NetFS-to-hpkg.patch (10.2 KB) - added by jalopeura 5 years ago.
0001-Converted-UserlandFS-and-NetFS-to-hpkg-and-made-nece.patch (31.9 KB) - added by jalopeura 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by mmadia

Blocked By: 10192 added

comment:2 Changed 5 years ago by jalopeura

Blocked By: 11168 added

Changed 5 years ago by jalopeura

comment:3 Changed 5 years ago by jalopeura

Has a Patch: set

comment:4 Changed 5 years ago by jalopeura

Notes:

  • Patches for #10192 and #11168 have already been submitted.
  • I don't know how to add a non-activated package, so it just follows the old method: you need to add NetFS as an optional package in your UserBuildConfig.
  • I did not verify that the files in the package work; I merely verfied that the package was created and that when I moved it into system/packages, the files showed up in the right places.

comment:5 Changed 5 years ago by bonefish

I don't really see a reason for removing sample settings files from the source tree. The UserlandFS settings for NetFS are needed. Otherwise the configuration (add/removing servers) won't work correctly. For support of proper packaging, UserlandFS needs to be adjusted to load settings differently.

  • It should first look in respective data subdirectories and load the settings from all files found there. Cf. how Expander loads its rules.
  • Then it should load the user settings (if any) just like before.

This way a package for a FS based on UserlandFS can simply include a settings file ("settings" is really not the correct word -- it's really more of an interface description) in the respective data subdirectory.

Another thing that should be changed in UserlandFS is where it looks for FS add-ons. IIRC it only looks in ~/config ATM. It should look in all four installation locations (system and home, packaged and non-packaged).

comment:6 Changed 5 years ago by bonefish

I forgot to add: The same comments as in comment:ticket:10192:7 wrt. build/jam/OptionalPackages and build/jam/repositories/Haiku apply.

comment:7 Changed 5 years ago by jalopeura

The advice I got when I asked about this on the devel list was that it should not include a settings file at all; instead the user should create whatever user settings files were needed.

I'm not sure a default settings file makes sense for NetFS, anyway, since the shares defined 1) may not exist, and 2) if they do, the default settings file opens a security hole.

I'm also not sure that including NetFS as a default in the UserlandFS settings makes sense. What if the user doesn't want NetFS and uses UserlandFS for something else? (On the other hand, it's not going to harm the user to have it in the default settings.)

I do think that checking multiple directories for settings files makes sense, though. The only problem I see is that NetFS uses BDriverSettings to load the file, and BDriverSettings doesn't work that way - there is one settings file per object instance.

If we're going to make a change like this, we should make it once, in the libroot functions that support BDriverSettings.

The relevant piece currently looks like this:

TODO: use B_SYSTEM_SETTINGS_DIRECTORY instead! if (find_directory(B_USER_SETTINGS_DIRECTORY, -1, false, path,

sizeof(path)) == B_OK)

We could change that to use the data directories instead.

On the other hand, if we only want to move UserlandFS add-on settings, but want regular driver settings to remain in the settings directory, then I could make a UserlandFSDriverSettings class that derives from BDriverSettings, and change the Load method to look in these other directories. Then UserlandFS and all add-ons can use that class instead of BDriverSettings.

It depends on whether this change in settings locations applies to all drivers or merely to UserlandFS add-ons.

To be honest, I don't really understand why you want it to be in a data directory instead of a settings directory. I'm not saying I'm unwilling to do it that way, I just don't see the reasoning behind it.

I mean, the package manager behavior will be the same whether we put it in settings or in data, right? It will keep or merge the old data depending on how we set up the package.

In any case, we're going to need some way to combine multiple settings files; perhaps a flag sent to parse_settings to tell it not to zero out the settings before parsing the data.

In fact, glancing through driver_settings.cpp, I don't see where it handles multiple parameters with the same name, which I suppose means if a file contains two instances of the same parameter, it will use the first one found.

comment:8 Changed 5 years ago by bonefish

We're talking about different "settings" files here. A settings file for NetFS itself -- i.e. the one defining shares and users -- shouldn't be included at all. The entry for NetFS in the UserlandFS settings file specifies information about NetFS's ioctl interface that UserlandFS needs to know to properly forward the ioctl data. So, strictly speaking, those aren't settings at all -- changing them would break certain functionality of NetFS. Let's call such an entry for a client FS an interface description instead.

It doesn't make much sense that the UserlandFS package includes the interface description for client FSs. Instead a package that contains a client FS should also provide the interface description for that client FS. There's a proven idiom to achieve something like that. It has e.g. already been implemented for Expander. Instead of a singleton expander rules file, Expander reads all files in the data/expander/rules directories of all installation locations. This way a package like xz_utils can provide a file data/expander/rules/xz_utils which contains the rules for xz packaged files.

In case of UserlandFS it can be implemented somewhat similarly. UserlandFS would scan the data/userlandfs/file_systems directories of all installation locations for a file matching a client FS. The NetFS package would provide its interface definition via a file data/userlandfs/file_systems/netfs. So, when UserlandFS wants to load the settings for NetFS, it would iterate through the installation locations (in order home non-packaged, home, system non-packaged, system) and read the first data/userlandfs/file_systems/netfs file it finds. It is not necessary to merge the content of multiple files.

Regarding your suggestion concerning the driver settings implementation. For kernel drivers there is usually no need to read more than one settings file. If a driver supports a settings file, it should be located under /system/settings/.... The user's home directory should not affect the kernel at all. We inherited that behavior from BeOS and it doesn't hurt ATM, since Haiku is essentially single-user, but that will change eventually. That's also what the TODO in load_driver_settings() is about.

Since in Haiku the driver settings API is also used by certain userland code, it may be useful to introduce functionality for reading all driver settings files with a certain name (or really all) from a list of directories. However, IMO that functionality would be more suitable for BDriverSettings than for the low level C API. (And it is not needed for UserlandFS nor for NetFS.)

Regarding your question concerning multiple parameters with the same name, the get_driver_*parameter() functions returns only the first occurrence. However, the API user can iterate through the driver settings structure manually and thus handle all occurrences, if necessary.

comment:9 Changed 5 years ago by jalopeura

This patch will also fix #10192 and #11168, since this one won't work without those two.

comment:10 Changed 5 years ago by pulkomandy

Resolution: fixed
Status: newclosed

Applied a reworked version of the patch in hrev48320.

Note: See TracTickets for help on using tickets.