Page MenuHomeFreeBSD

FFclock: renaming of the FFclock daemon API structure
AcceptedPublic

Authored by Darryl.Veitch_uts.edu.au on Dec 7 2023, 12:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 21, 4:45 AM
Unknown Object (File)
Tue, Feb 13, 1:46 AM
Unknown Object (File)
Jan 18 2024, 1:23 PM
Unknown Object (File)
Jan 5 2024, 10:39 PM
Unknown Object (File)
Dec 24 2023, 1:37 AM
Unknown Object (File)
Dec 22 2023, 1:41 AM
Unknown Object (File)
Dec 18 2023, 2:49 AM
Subscribers

Details

Reviewers
phk
imp
brooks
Summary

Context: The struct ffclock_estimate member of struct fftimehands holds
the parameters maintained by the FFclock daemon for the native FFclock
(natFFC). It is poorly named as it could be misinterpreted as a mere
timestamp. It is in fact the fundamental data that DEFines natFFC and
allows it to be read at any time.

This commit renames the structure and the variable name typically used
with it, from

struct ffclock_estimate cest;

to

struct ffclock_data cdat;

and similarly in other cases. Some matching "estimates" --> "data"
comment changes are also made.

Syscall impacts:
The name change impacts two system calls, renamed as :

ffclock_{get,set}estimate --> ffclock_{get,set}data

Though the code is functionally identical, technically they are new
syscalls as the symbols (names) have changed, as has the name of their
argument's type.
Required changes:

syscalls.master    (for autogen of syscallname_args)
 - obsoleting of old syscalls (names and numbers retained)
 - new calls added at end  (588, 589)
 - ** must "make sysent" in /usr/src to activate the autogen of
   dependent syscall files before compilation!

Makefile.inc       (for libc linkage to userland programs)
 - update syscall names to the new ones
Symbol.map         (versioned syscall symbol name database)
 - adding new entry FBSD_1.8 suitable for new calls under 15.0-CURRENT
 - including the new calls under this entry

Though in essence only a name change, it is isolated here as it is
quite noisy, to avoid obscuring substantive code changes. Though
technically it is a change of API, ffclock_version is not bumped.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 55042
Build 51931: arc lint + arc unit

Event Timeline

brooks requested changes to this revision.Dec 7 2023, 7:29 PM

I see no reason to change the system call numbers when there is no ABI change. You'll need to add entries to lib/libc/include/compat.h to expose the old symbols under the right version for compatibility.

(If you were to add new numbers you'd also have to add COMPAT14 support for the old syscall numbers and I don't recommend that as it's a pain.)

lib/libc/sys/Makefile.inc
406

I'd probably retain the old MLINKS and update HISTORY

lib/libc/sys/Symbol.map
360–361

Need to remove these.

This revision now requires changes to proceed.Dec 7 2023, 7:29 PM

Hi Brooks. Am very happy to remove the need to change syscalls numbers, did not realise that was possible when the symbol itself (the name) changed. As suggested I removed the old symbols from Symbol.map, put the new names under the original numbers in syscalls.master, and to compat.h added:
sym_compat(ffclock_setestimate, freebsd15_ffclock_setdata, FBSD_1.3);
sym_compat(ffclock_getestimate, freebsd15_ffclock_getdata, FBSD_1.3);
which was the only option that made sense to me that was consistent with existing entries. Does this look right?

This compiles and boots and the new syscalls work, but the backward compatibility does not: my old version of radclock (using the old symbols) is not recognising the old symbols. I did a world build to ensure that libc libraries were aware of the latest.
I also tried FBSD_1.0 at the end, same result, and in desperation reverted that and also returned the old
symbols to Symbol.map under FBSD_1.3, with a world build each time. No luck.
Either my entries above are wrong or there is something else I am missing.

Hi Brooks. Am very happy to remove the need to change syscalls numbers, did not realise that was possible when the symbol itself (the name) changed. As suggested I removed the old symbols from Symbol.map, put the new names under the original numbers in syscalls.master, and to compat.h added:
sym_compat(ffclock_setestimate, freebsd15_ffclock_setdata, FBSD_1.3);
sym_compat(ffclock_getestimate, freebsd15_ffclock_getdata, FBSD_1.3);
which was the only option that made sense to me that was consistent with existing entries. Does this look right?

There shouldn't be a freebsd15_ prefix. The above would simply make the ffclock_* symbols undefined. The reason there is a prefix for others is that the freebsd#_ functions are compatibility shims that call the new syscall implementation with different ABIs.

Thanks for that. I have it working now. I am ready to "git arc update" if you are happy with my response to your comment regarding the man (2) page

lib/libc/sys/Makefile.inc
406

Since the old calls, I believe, are not currently used, there is no user base
(application or human) that needs to be alerted of the change. Hence I think is is best to
simply use the new calls, referencing unused old ones would not help anyone.

Thanks for that. I have it working now. I am ready to "git arc update" if you are happy with my response to your comment regarding the man (2) page

Please do.

lib/libc/sys/Makefile.inc
406

Works for me on the MLINKS front. I think an entry in HISTORY might still be a good idea. Probably just something as simple as the following, maybe with a sentence of explanation. It's up to you though as I agree these are generally internal-use interfaces.

The
.Fn ffclock_getdata
and
.Fn ffclock_setdata
system calls where named
.Fn ffclock_getestimate
and
.Fn ffclock_setestimate
prior to
.Fx 15 .

Reusing existing syscall numbers
Removal of new syscall numbers, reusing old ones instead, with
associated changes to ensure backward compatibility, as per discussion.

I have resubmitted with the addition to HISTORY you suggested.

This seems fine now, but I do worry that D42940 may require a new syscall (or perhaps compatibility based on p_osreal)

This revision is now accepted and ready to land.Dec 20 2023, 6:38 PM