[libvirt] [PATCH 18/41] remote: in per-driver daemons ensure that state initialize succeeds

[Daniel P. Berrangé]

On Fri, Jul 26, 2019 at 08:25:05PM +0200, Andrea Bolognani wrote:
> On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
> [...]
> > @@ -648,15 +650,23 @@ virStateInitialize(bool privileged,
> [...]
> > +            if (ret == VIR_DRV_STATE_INIT_ERROR) {
> >                  VIR_ERROR(_("Initialization of %s state driver failed: %s"),
> >                            virStateDriverTab[i]->name,
> >                            virGetLastErrorMessage());
> >                  return -1;
> > +            } else if (ret == VIR_DRV_STATE_INIT_SKIPPED &&
> > +                       mandatory) {
> 
> You can fit this entire condition on a single line.
> 
> [...]
> > +++ b/src/remote/remote_daemon.c
> > @@ -794,6 +794,11 @@ static void daemonRunStateInit(void *opaque)
> >       * we're ready, since it can take a long time and this will
> >       * seriously delay OS bootup process */
> >      if (virStateInitialize(virNetDaemonIsPrivileged(dmn),
> > +#ifdef MODULE_NAME
> > +                           true,
> > +#else /* ! MODULE_NAME */
> > +                           false,
> > +#endif /* ! MODULE_NAME */
> >                             daemonInhibitCallback,
> >                             dmn) < 0) {
> 
> Just like in patch 10, this is really ugly... Please change it to
> something like
> 
>   #ifdef MODULE_NAME
>     bool mandatory = true;
>   #else /* ! MODULE_NAME */
>     bool mandatory = false;
>   #endif /* ! MODULE_NAME */
> 
>   virStateInitialize(virNetDaemonIsPrivileged(dmn),
>                      mandatory,
>                      daemonInhibitCallback,
>                      dmn);
> 
> [...]
> > +++ b/src/vz/vz_driver.c
> > @@ -4118,36 +4118,36 @@ vzStateInitialize(bool privileged,
> [...]
> >      /* Failing to create driver here is not fatal and only means
> >       * that next driver client will try once more when connecting */
> >      vz_driver = vzDriverObjNew();
> > -    return 0;
> > +    return VIR_DRV_STATE_INIT_COMPLETE;
> 
> Given the comment, are you sure we shouldn't do something like
> 
>   if (!(vz_driver = vzDriverObjNew()))
>     return VIR_DRV_STATE_INIT_SKIPPED;
> 
>   return VIR_DRV_STATE_INIT_COMPLETE;
> 
> here instead?

Marking it as skipped would cause the daemon to exit which against
the semantics that the vz driver code was trying to achieve with
this startup behaviour. 

> With the nits above addressed, and assuming the logic in the vz
> driver either is confirmed to be fine as or is changed appropriately,
> 
>   Reviewed-by: Andrea Bolognani <abologna at redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|