On Tue, 22 Aug 2017 10:56:31 +0100
Christopher Baines <mail@cbaines.net> wrote:

> On Mon, 21 Aug 2017 19:38:25 -0700
> Ryan Moe <ryan.moe@gmail.com> 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!

I've now pushed the 3 patches to master :)