[bug#27887] [PATCH] services: Add libvirt services

[Christopher Baines]

On Mon, 21 Aug 2017 19:38:25 -0700
Ryan Moe <address@hidden> wrote:

> I did some more testing and everything looks good to me. I've
> addressed your previous comments as well (see below). I wasn't sure if
> I needed to squash the commits so I just included all three patches. I
> did reorder them to place your wrap patch before mine since the
> libvirt service depends on that change.

Awesome, that all sounds perfect.

> >> +(define libvirt-service-type
> >> +  (service-type (name 'libvirt)
> >> +             (extensions
> >> +                 (list
> >> +                  (service-extension polkit-service-type (compose
> >> list libvirt-configuration-libvirt))  
> >
> > Line length could be better here, just by putting the (compose ... )
> > bit on the line after the polkit-service-type.
> >  
> 
> Agreed. This is fixed in the attached patch.
> 
> >> +                  (service-extension profile-service-type
> >> +                                     (compose list
> >> +                                           (lambda (package) qemu)
> >> +
> >> libvirt-configuration-libvirt))  
> >
> > This confused me for a bit, until I realised that a simpler way of
> > expressing this would be (const (list qemu)) if I'm correct? Also,
> > it would be good to explain why this needs to happen in a comment.
> >  
> 
> That didn't work. qemu doesn't need to be installed in the system
> profile anyway so I've removed this.
> 
> >> +(define virtlog-service-type
> >> +  (service-type (name 'virtlogd)
> >> +             (extensions
> >> +                 (list
> >> +                  (service-extension profile-service-type
> >> +                                     (compose list
> >> +
> >> virtlog-configuration-libvirt))  
> >
> > What function does this extension have? As far as I understood from
> > the documentation you wrote, this is used from the libvirt
> > service.  
> 
> Now that I have a better understanding of how this works I realize
> this was unnecessary and it's been removed too.

I'll plan on merging this tomorrow then, leaving a little bit of time
for anyone else to review this if they want to. Great work Ryan!

pgpmLSdY7FTd6.pgp
Description: OpenPGP digital signature