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

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?

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(a)redhat.com&gt;