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@xxxxxxxxxx>
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
