Scan the source tree and use find_directory() where appropriate (no hard-coded paths anymore)

[kaoutsis]

with style changes

[kaoutsis]

only with the find_directory changes

[kaoutsis]

Comments about the src_preferences_screensaver_ScreenSaverApp.cpp.diff: Actually there are modifications in two files: a) src/preferences/screensaver/ScreenSaverApp.cpp b) src/preferences/screensaver/ScreenSaverWindow.cpp Besides the find_directory fix, i take the freedom to add an enhancement: Now the ScreenSaver pref. can receive more than one ref at a time; so you can add one or more screensavers with the 'add' button or you can simply drag and drop the screensavers into the ScreenSaver window :-)

[kaoutsis]

Comments about the src_preferences_screensaver_ScreenSaverApp.cpp.diff: Τhe ScreenSaver pref. is now capable to distinguish the real screensavers and installs only them to the user addon directory, when the user drop files in the ScreenSaver pref. window; tested successfully with the collection of screensavers attached to #511

[korli]

HWindow.cpp, TrackerInitialState.cpp and FontManager.cpp patched

[kaoutsis]

Replying to korli:

HWindow.cpp, TrackerInitialState.cpp and FontManager.cpp patched

May be TrackerInitialState.cpp is forgotten?

[kaoutsis]

comments on src_bin_desklink_desklink.cpp.diff:

• the usual find_directory() fix
• i took the freedom, to add a 'About desklink...' menu item to the pop up menu.

But i used the asynchronous Go() version (with the NULL argument); The synchronous version ate all the cpu, but i don't know if that is a bug, since the alert was inside the MessageReceived() loop. The asynchronous Go() version seems to work fine, though.

[korli]

Applied desklink.cpp.diff in hrev25462. I factorized the alert code and change a few things.

[kaoutsis]

src_system_kernel_main.cpp.diff: updated to hrev25481

[kaoutsis]

src_preferences_screensaver_ScreenSaverApp.cpp.diff:

• fixed a typo, that i introduced to the 'check for a screensaver' routine:

the buffer that is used to contain the informations in order to examine if the file is a screensaver was not use memset correctly (only the first 4096 bytes were filled with 'a', not the entire buffer); that caused a bug: in some rare cases, the buffer when is reused, may contains the string 'StartSaver', that was left there from the examination of a previous real screensaver file, since these addresses would be part of the program's free address space, resulting in a wrong decision about the file in question. The fix makes now the routine more efficient: it doesn't use memset to fill the whole buffer with 'a' every time that the buffer is initialized, but only at the point where it finds a screensaver file, it fills the specific positions of the string with 'a', thus memset is using only 10 bytes.

[stippi]

I am looking into the remaining patches. They all look pretty good, I am just thinking that the Append() calls need error checking and for some of these, I consider using hardcoded paths as fallbacks.

[axeld]

I don't know if this is really necessary - if Append() already fails, there doesn't seem to be much memory left to do anything useful, anyway :-) Well, the stuff in the kernel might do so.

[stippi]

devfs.cpp.diff applied in hrev25503.

[stippi]

team.cpp.diff applied in hrev25506.

[stippi]

system_kernel_main.cpp.diff applied in hrev25509.

[korli]

About src_preferences_screensaver_ScreenSaverApp.cpp.diff, I'm not sure the way the 'check for a screensaver' is done is good. Why not let the screensaver loading do its job ?

[bonefish]

I'm actually not quite sure, whether I find it a good idea to use find_directory() in system components like the kernel or the runtime loader. They just complicate things without real benefit. I mean, the whole point of using find_directory() in applications is to still work as expected when the system's directory layout changes. But these components *are* the system. I think I'd rather use a common single header file that defines the paths.

[axeld]

That common single header sounds like a good idea. It should then be used by the kernel and find_directory() itself (if viable). find_directory() would only make sense in the kernel as well, if we make these paths configurable - which I don't think is really necessary.

[kaoutsis]

Replying to korli:

About src_preferences_screensaver_ScreenSaverApp.cpp.diff, I'm not sure the way the 'check for a screensaver' is done is good. Why not let the screensaver loading do its job ?

I'm not sure, either :-)

There are two issues here: 1) The basic idea was that the screensaver list should contains only screensaver files.

Imagine the scenario: a user drops a readme file, and that readme appears in the screensaver list. If we can prevent that, i think that to check for a valid screensaver, is a good idea. Of course, if we want the screensaver list to contain only screensaver files, there is more work to be done. For example, the user may drop the file(s) directly to the addons directory, then the screensaver list should updated with only the valid screensaver files, and if the user insist to drop the file in the ScreenSaver window, he will get the alert info: file x, is not a screensaver file.

2) Now the 'routine' check: Obviously, the routine is some kind of practical algorithm :-) All the screensaver-binary-files that i could tested contained the 'StartSaver' string somewhere inside a 80k limit (actually the real limit is somewhere between 40k, i increased to 80k just in case). Of course, a valid screensaver should use the StartSaver() function (i hope this hypothesis is correct). Giving the fact, that the user hopefully, will drop screensaver files, the routine checks first the first 4096 bytes, then 4096 * 2, then 4096 * 3, and so on; it usually finds the screensaver after 4 or 5 tries. Now if the user drops a irrelevant file then indeed the check hits the limits (n = 20 times and each time checks 4096 * 1, 4096 * 2, ...,4096 * n bytes).

If you think that 1. is a good idea, then we can think over 2. It's up to you :-)

[korli]

Replying to kaoutsis:

If you think that 1. is a good idea, then we can think over 2.

I'm OK for 1. I think 2. is not the way to go. I would prefer the actual code to load the addon:

image = load_add_on(path);
if (image != B_OK)
   ...
if (get_image_symbol((image, "instantiate_screen_saver", B_SYMBOL_TYPE_TEXT, (void **)&instantiate) != B_OK)
   ...
unload_add_on(image);

Isn't it better?

[kaoutsis]

Replying to korli:

Replying to kaoutsis:

If you think that 1. is a good idea, then we can think over 2.

I'm OK for 1. I think 2. is not the way to go. I would prefer the actual code to load the addon:

image = load_add_on(path); if (image != B_OK)

...

if (get_image_symbol((image, "instantiate_screen_saver", B_SYMBOL_TYPE_TEXT, (void )&instantiate) != B_OK)

...

unload_add_on(image);

Isn't it better?

I believe yes. I will try an implementation based upon this idea; i will take my time, and i will come back later with an updated patch, OK?

[kaoutsis]

src_preferences_screensaver_ScreenSaverApp.cpp.2.diff:

There are a number of changes in order to make the ScreenSaver pref. contains only screensavers in the list, notably:

1) The use of load_add_on() and get_image_symbol() functions.

Note however that there is a difference in relation with the previous implementation: Besides the really irrelevant files, the routine now 'cuts off' also screensaver files that can't loaded correctly, there are two cases: a) screesaver-files with missing library, e.g libGLU.so b) screensaver-files with unresolved symbols, e.g elf_resolve_symbol: could not resolve symbol 'ReadFrames11BMediaTrackPcPxP12media_header'

2) The use of watch_node() in order to monitor the user's Screen Savers addons directory. Since we are moving to a multiuser environment, i think there isn't need to monitor other dirs, the user would access these dirs read-only.

• Although the list is updated with only the valid screensaver files

on the fly, namely when the ScreenSaver pref. is running and the user drops the file(s) directly to the addons directory, the ScrenSaver pref. crashes is some cases if the user selects some item from the list; i still don't know why and where.

[kaoutsis]

src_servers_registrar_TRoster.cpp.diff:

• removed unused variable

static const char *const kVitalSystemAppPathPrefix

• use of find_directory() & BPath & co.
• use of D(PRINT()) macro to return errors

[kaoutsis]

src_servers_debug_DebugServer.cpp.diff:

• updated to hrev25560
• also take into account the returned status values

of BPath::Append()

[kaoutsis]

src_apps_processcontroller_PCWorld.cpp.diff:

• find_directory() and friends.

[leavengood]

I will add the ScreenSaver diff, this looks pretty useful and I am already working on the ScreenSaver preferences anyhow.

[scottmc]

Have all of these patches been applied yet? Are there more to come?

[nielx]

No discussion since 10 years. With the package management, it should have been obvious if some applications on the main image have been doing this wrong.

Closing for now.

[nielx]

Assign tickets with status=closed and resolution=fixed within the R1/beta2 development window to the R1/beta2 Milestone