guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug#27855] [PATCH] gnu: Add rsync service.


From: Christopher Baines
Subject: [bug#27855] [PATCH] gnu: Add rsync service.
Date: Sat, 19 Aug 2017 23:19:49 +0100

On Sat, 19 Aug 2017 23:34:20 +0300
Oleg Pykhalov <address@hidden> wrote:

> Hello Christopher,
> 
> Christopher Baines <address@hidden> writes:
> 
> >> > Yep, I think I just stopped writing the test after finding the
> >> > issue with the PID file.
> >> >
> >> > I haven't looked in to how to fix this in the test, so if you
> >> > could, that would be great. Otherwise, I'll probably have time
> >> > to look at this again within a week or so.
> >> >
> >> > You'll probably need to refactor the test a bit, as at the
> >> > moment, the information regarding the port isn't available where
> >> > you run the commands.    
> >> 
> >> Of course I'll try.  By the way, how to run a “vm”?  Previous
> >> method “./pre-inst-env guix system vm gnu/tests/rsync.scm” doesn't
> >> work for me.  
> >
> > I'm guessing that you'll need to make the file evaluate (I'm not
> > sure if that is the right word here) to an operating-system, e.g.
> > put %rsync-os-with-port-2000 right at the bottom of the file, and
> > then guix system vm should give you a start script that will start
> > a VM for that OS.  
> 
> I did some work on rsync service:
> 
> - Fixed PID and synchronization to specific port.
> - Merged two rsync oses in one with optional port.
> - Added ports to rsync synchronization tests and change protocol from
>   ssh to rsync.
> - Added some logic to config: chroot (can use only root), user and
>   group.
> 
> All tests passed successfully for me.

Great :) Now that the tests pass at least, I don't see any reason to
not merge this soonish.

I've still done some thingking about how the configuration works
though, and I've been considering a few ways of tweaking this so that
its harder to break, and clear in how it works.

One way I've managed to break the service so far is setting the user
and group to root in the configuration. This causes the tests to fail,
and in a odd way, as I think the problem is that the creation of
the /var/run/rsync directory relies on the account service, but I'm
guessing that the account service does nothing in this case, as root is
already an account.

One way of making this harder to break would be to explicitly create
the necessary directories when this service is activated, e.g.:

  (mkdir-p (dirname #$(rsync-configuration-pid-file config)))
  (mkdir-p (dirname #$(rsync-configuration-lock-file config)))

I think there is also the opportunity to make the service configuration
clearer here, as considering the default port test, the default
configuration says it will run as the rsync user, but the service will
actually run as the root user.

This could be improved by making the configuration more uncertain by
default, e.g. user defaults to #f, which means the correct user is
decided based on the port.

Also on the subject of clarity, the use-chroot? option is something
that can be specified, but the value might not be used. My preference
would be to change the "logic" in the configuration file generation, to
validation, e.g.:

  (if (and use-chroot? (not (eq? user "root")))
      (error "rsync-service: to run rsync in a chroot, the user must be root"))

The use-chroot? option might also benefit from making the user default
to #f, as then the service could decide the user based on the port and
use-chroot? settings, without contradicting the configuration.

Attachment: pgpmIJILtIRAw.pgp
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]