[no subject]

Couple more comments on the patch are below.

> > Changes since version 1
> > 1. Fix variable/struct/function names as suggested
> > 2. Avoid calling LOG_FATAL from the helper
> > 3. Added #define PRIVOPS_xxxxx for each privileged function
> > 4. Transfer fd request and data in single I/O
> > 5. helper_main() restructured as suggested.
> > 6. remove privops.o from Makefile.in

Nice.

> > +#include "sysincl.h"
> > +#include "conf.h"
> > +#include "logging.h"
> > +#include "privops.h"
> > +#include "util.h"
> > +
> > +#if PRIVOPS_ANY

If the file will be compiled only when the helper is needed, this #if
doesn't have to be here.

> > +typedef struct {
> > +  int sock;

This field probably shouldn't be here as it's sent in the control message.

> > +  socklen_t sa_len;
> > +  union sockaddr_in46 sa;
> > +} ReqBindSocket;

> > +static int
> > +send_with_fd(int fd, int fd_to_send, const void *data, int data_len)

Instead of hardcoding the case with sending socket over SCM_RIGHTS
control message, could this function take a pointer to cmsghdr?
Functions that prepare requests, but don't need a control message, would
pass NULL.

> > +  cmsg->cmsg_level = SOL_SOCKET;
> > +  cmsg->cmsg_type = SCM_RIGHTS;
> > +  cmsg->cmsg_len = CMSG_LEN(sizeof(int));
> > +
> > +  ptr_send_fd = CMSG_DATA(cmsg);
> > +  memcpy(ptr_send_fd, &fd_to_send, sizeof(int));

Can this be a direct assignment?

> > +static int
> > +receive_with_fd(int fd, void *data, int data_size, int *fd_recvd)

Here it would be a pointer to a cmsg buffer.

> > +  /* extract transferred descriptor (if any) */
> > +  for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> > +    if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
> > +      memcpy(fd_recvd, CMSG_DATA(cmsg), sizeof(int));
> > +  }

This would be in do_bindsocket().

> > +/* HELPER - send response to the fd */
> > +
> > +static inline int

Why inline? I'd leave that up to the compiler.

> > +static void
> > +do_adjtime(const PrvRequest *req, PrvResponse *res)

Wouldn't the code be a bit more readable if req was the operation
specific request type? Too bad the response has to be the generic type
to allow returning the fatal error.

> > +{
> > +  const struct timeval *delta = &req->u.adj_tv.tv;
> > +  struct timeval olddelta;
> > +
> > +  memset(res, 0, sizeof(PrvResponse));

memset() calls can be moved to the main function.

> > +  while (1) {
> > +    if (receive_with_fd(fd, &req, sizeof(PrvRequest), &recvd_fd) < sizeof(PrvRequest))
> > +      /* read error or closed input - we cannot recover - give up */
> > +      return;
> > +
> > +    switch (req.op) {
> > +    default:
> > +      res_fatal(&res, "Unexpected operator : %d", req.op);
> > +      break;

Can you please move the default case to the end of the switch
statement? I didn't even know this syntax was valid :).

> > +    case op_BINDSOCKET:
> > +      req.u.bind_sock.sock = recvd_fd; /* use transferred descriptor */
> > +      do_bindsocket(&req, &res);

There would be a pointer to the cmsg buffer as an extra argument.

> > +PRV_BindSocket(int sock, struct sockaddr *address, socklen_t address_len)
> > +{
> > +  PrvRequest req;
> > +  PrvResponse res;
> > +  IPAddr ip;
> > +  unsigned short port;
> > +
> > +  if (address->sa_len > address_len)

On some systems there is no sa_len in struct sockaddr. Does it have to
be checked?

> > +void
> > +PRV_Initialise(void)
> > +{
> > +  pid_t pid;
> > +  int sock_pair[2];
> > +
> > +  if (have_helper())
> > +    LOG_FATAL(LOGF_PrivOps, "PRV_start, helper is already running");
> > +
> > +  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock_pair) != 0)

Shouldn't this actually be SOCK_DGRAM to preserve message boundaries?
Or is that not a problem with such small messages? I suspect it
would be more difficult to detect that the other end of the socket was
closed. SOCK_SEQPACKET doesn't seem portable.

> > +    /* close other descriptors inherited from the parent process */
> > +    for (fd = 0; fd < 1024; fd++) {
> > +      if (fd != sock_pair[1])
> > +        close(fd);
> > +    }
> > +
> > +    helper_main(sock_pair[1]);
> > +
> > +    close(sock_pair[1]);
> > +    exit(0);

The two lines could be moved to helper_main().

> > +PRV_Finalise(void)
> > +{
> > +  int status;
> > +  close(helper_fd);
> > +  helper_fd = -1;
> > +  wait(&status);

wait() seems to require <sys/wait.h>.

-- 
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.