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

26 Jul 2019


      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@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

    