Re: [RFC PATCH v2 4/4] Add rtc server

[Zhaoming Luo]

On 11/8/24 5:10 PM, Sergey Bugaev wrote:
> On Fri, Nov 8, 2024 at 4:55 AM Zhaoming Luo <zhmingluo@163.com> wrote:
> > diff --git a/rtc/main.c b/rtc/main.c
> > new file mode 100644
> > index 00000000..114fb497
> > --- /dev/null
> > +++ b/rtc/main.c
> > @@ -0,0 +1,88 @@
> > +
> > +#include <version.h>
> > +
> > +#include <error.h>
> > +#include <argp.h>
> > +#include <nullauth.h>
>
> I don't think you need nullauth.h?
Indeed, removed it.
> > +#include <hurd/trivfs.h>
> > +#include <hurd/ports.h>
> > +#include <hurd/rtc.h>
> > +
> > +#include "rtc_pioctl_S.h"
> > +
> > +const char *argp_program_version = STANDARD_HURD_VERSION (rtc);
> > +
> > +struct trivfs_control *rtccntl;
>
> Please make global variables 'static' unless there is a reason not to.
Got it.
> > +int trivfs_fstype = FSTYPE_DEV;
> > +int trivfs_fsid = 0;
> > +int trivfs_support_read = 0;
>
> Actually Linux lets you read() /dev/rtc, doesn't it?
Is it implied by 'it is read-only' in [0]?
[0]:https://man.archlinux.org/man/rtc.4
> > +int trivfs_support_write = 0;
> > +int trivfs_support_exec = 0;
> > +int trivfs_allow_open = O_READ | O_WRITE;
> > +
> > +static const struct argp_option options[] =
> > +{
> > +  {0}
> > +};
> > +
> > +/* TODO: adding option */
>
> Hm, so what kind of options do you envision it might have? (Not saying
> there's none, just wondering.)
To be honest options was added just in case I want to add some options, 
but I haven't got any idea.
> Can there be multiple RTC devices? (the Linux man page talks of
> "/dev/rtc0, /dev/rtc1, etc."). Do we need a way to identify which
> specific RTC device the translator should drive?
I found some discussion about 'multiple RTC devices" on [1]. It is 
possible to have multiple RTC devices on a single machine.

I think we exactly need a way to identify RTC devices (though I have no 
idea how to do that). Different RTC device needs different 
`pioctl-ops.c` (i.e. driver) to access. How about make each 
implementation a package? Then users can install the drivers as packages 
depending on their needs?
[1]:https://www.kernel.org/doc/html/v6.12-rc6/admin-guide/rtc.html
> > +static error_t
> > +parse_opt (int opt, char *arg, struct argp_state *state)
> > +{
> > +  return ARGP_ERR_UNKNOWN;
> > +}
> > +
> > +static const struct argp rtc_argp =
> > +{ options, parse_opt, 0, "RTC device" };
> > +
> > +int
> > +demuxer (mach_msg_header_t *inp, mach_msg_header_t *outp)
>
> Same here, make it 'static' please, unless there is a reason not to.
OK
> > +{
> > +  mig_routine_t routine;
> > +  if ((routine = rtc_pioctl_server_routine (inp)) ||
> > +      (routine = NULL, trivfs_demuxer (inp, outp)))
> > +    {
> > +      if (routine)
> > +        (*routine) (inp, outp);
> > +      return TRUE;
> > +    }
> > +  else
> > +    return FALSE;
> > +}
> > +
> > +int
> > +main (int argc, char **argv)
> > +{
> > +  error_t err;
> > +  mach_port_t bootstrap;
> > +
> > +  argp_parse (&rtc_argp, argc, argv, 0, 0, 0);
> > +
> > +  task_get_bootstrap_port (mach_task_self (), &bootstrap);
> > +  if (bootstrap == MACH_PORT_NULL)
> > +    error (1, 0, "Must be started as a translator");
> > +
> > +  /* Reply to our parent */
> > +  err = trivfs_startup (bootstrap, 0, 0, 0, 0, 0, &rtccntl);
>
> Even more of a nitpick: better pass NULL instead of 0 when the type is
> a pointer. So:
>
> trivfs_startup (bootstrap, 0, NULL, NULL, NULL, NULL, &rtccntl)
OK
> you might even specify the first 0 as O_NORW for clarity, but I
> haven't seen anyone do that.
We can start doing it now :).
> > diff --git a/rtc/pioctl-ops.c b/rtc/pioctl-ops.c
> > new file mode 100644
> > index 00000000..44038e26
> > --- /dev/null
> > +++ b/rtc/pioctl-ops.c
> > @@ -0,0 +1,28 @@
> > +/* Server side implementation for rtc device */
> > +
> > +/* This implementation is largely based on sys-utils/hwclock from util-linux */
> > +
> > +#include "rtc_pioctl_S.h"
> > +#include <hurd/rtc.h>
> > +#include <hurd/hurd_types.h>
> > +
> > +/* 9 RTC_RD_TIME -- Read RTC time */
> > +kern_return_t
> > +rtc_S_pioctl_rtc_rd_time (struct trivfs_protid *cred, struct rtc_time *time)
> > +{
> > +  if (!cred) {
> > +    return EOPNOTSUPP;
> > +  }
>
> Nitpick: according to the GNU C style, this pair of braces shouldn't be here.
OK
> > +  if (!(cred->po->openmodes & O_READ))
> > +    return EBADF;
> > +  return KERN_SUCCESS;
> > +}
> > +
> > +/* 10 RTC_SET_TIME -- Set RTC time */
> > +kern_return_t
> > +rtc_S_pioctl_rtc_set_time (struct trivfs_protid *cred, struct rtc_time time)
> > +{
> > +  if (!(cred->po->openmodes & O_WRITE))
> > +    return EBADF;
>
> You need to check for cred being NULL here too, before you dereference it.
OK
> Again, this happens when the request arrives on some port that we
> listen for messages on, but it corresponds to something other than a
> protid, e.g. the trivfs control port. EOPNOTSUPP is the appropriate
> error code to return in that case, because indeed the
> whatever-other-kind of port doesn't support the ioctl, only protids
> do.
>
> > +  return KERN_SUCCESS;
> > +}
Thank you for the code view.

--
Zhaoming Luo