Re: [chrony-dev] [PATCH] refclock: add SENSORS driver for OpenBSD timedelta sensors

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

On Tue, Jul 14, 2026 at 12:12:45AM +0300, Atanas Vladimirov wrote:
> This patch adds a new refclock driver for OpenBSD, which reads timedelta sensors from the hw.sensors sysctl framework.

> Until now (last few months since I test it) the only way to use these sensors with chronyd was an external bridge program feeding the SOCK refclock. 
> A native driver removes that extra daemon and process boundary.

Was that a big problem using that bridge? The code looks good (few
comments below), but I hesitate to add and maintain a driver that
will be probably used only by a very small number of users, and that
I cannot even test. I have OpenBSD only as a VM. If it was accepted,
would you stay around and keep an eye on it?

> --- a/configure
> +++ b/configure

> +if [ $feat_refclock = "1" ] && \
> +  test_code 'OpenBSD sensors' 'sys/types.h sys/time.h sys/sysctl.h sys/sensors.h' '' '' '
> +    struct sensordev sd;
> +    struct sensor s;
> +    int mib[5] = { CTL_HW, HW_SENSORS, 0, SENSOR_TIMEDELTA, 0 };
> +    size_t len = sizeof (s);
> +    return sysctl(mib, 5, &s, &len, NULL, 0) + sizeof (sd.xname) +
> +           SENSOR_S_OK + SENSOR_FINVALID;'
> +then
> +  add_def FEAT_SENSORS
> +fi

This check should run only on OpenBSD (e.g. add a try_sensors
variable).

> @@ -636,6 +636,24 @@ Examples:
>  refclock RTC /dev/rtc0:utc
>  ----
>  +
> +*SENSORS*:::
> +Driver for timedelta sensors provided by the OpenBSD *hw.sensors* framework.

Is the driver name "SENSORS" in plural form not confusing a bit here?
I'd prefer shorter "SENSOR" if you think that is acceptable.

> +static int read_timedelta(int dev, const char *name, struct sensor *s)
> +{
> +  struct sensordev sd;
> +  int mib[5] = { CTL_HW, HW_SENSORS, dev, SENSOR_TIMEDELTA, 0 };
> +  size_t len = sizeof (*s);

Just a cosmetic nit, the decleration of "sd" should be the last one
here as shortest.

> +static int sensors_initialise(RCL_Instance instance)
> +{
> +  struct sensors_instance *sensors;
> +  struct sensor s;
> +  char *name;
> +  int dev;
> +
> +  RCL_CheckDriverOptions(instance, NULL);
> +
> +  name = RCL_GetDriverParameter(instance);
> +  if (!name || name[0] == '\0')
> +    LOG_FATAL("Missing sensor device");
> +
> +  dev = find_sensordev(name);
> +  if (dev < 0)
> +    LOG_FATAL("Could not find sensor device %s", name);

This should be rebased for the recent changes made for the
"optional" refclock option.

> +  if (read_timedelta(dev, name, &s) < 0)
> +    LOG_FATAL("Could not read timedelta sensor of device %s", name);
> +
> +  sensors = MallocNew(struct sensors_instance);
> +  sensors->dev = dev;
> +  sensors->name = name;

Better to Strdup() the name here in case it moves to stack, or is
reused/modified for some reason.

-- 
Miroslav Lichvar


-- 
To unsubscribe email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "unsubscribe" in the subject.
For help email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "help" in the subject.
Trouble?  Email listmaster@xxxxxxxxxxxxxxxxxxxx.