On Fri, Nov 8, 2024 at 4:55 AM Zhaoming Luo <zhming...@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?
> +#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.
> +int trivfs_fstype = FSTYPE_DEV;
> +int trivfs_fsid = 0;
> +int trivfs_support_read = 0;
Actually Linux lets you read() /dev/rtc, doesn't it?
> +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.)
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?
> +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.
> +{
> + 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)
you might even specify the first 0 as O_NORW for clarity, but I
haven't seen anyone do that.
> 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.
> + 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.
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;
> +}
Sergey
