Re: [libvirt] [PATCH v3 19/48] remote: in per-driver daemons ensure that state initialize succeeds
On Tue, 2019-07-30 at 12:59 +0200, Christophe de Dinechin wrote:
> Daniel P. Berrangé writes:
[... 163 lines removed ...]
> > @@ -648,15 +650,22 @@ virStateInitialize(bool privileged,
> >
> > for (i = 0; i < virStateDriverTabCount; i++) {
> > if (virStateDriverTab[i]->stateInitialize) {
> > +virDrvStateInitResult ret;
> > VIR_DEBUG("Running global init for %s state driver",
> >virStateDriverTab[i]->name);
> > -if (virStateDriverTab[i]->stateInitialize(privileged,
> > - callback,
> > - opaque) < 0) {
> > +ret = virStateDriverTab[i]->stateInitialize(privileged,
> > +callback,
> > +opaque);
> > +VIR_DEBUG("State init result %d (mandatory=%d)", ret,
> > mandatory);
> > +if (ret == VIR_DRV_STATE_INIT_ERROR) {
>
> I'm a bit conflicted here. I like the explicit "error" in the name, but
> all the code checks for errors with < 0, and that would work here too.
> But then, you also just replied to me that libvirt only uses -1 as an
> error value, so the < 0 really means == -1... Not sure what to prefer
> here ;-)
Most functions in libvirt either succeed (0) or fail (-1), but in
some cases we need to be able to tell apart different reasons for the
failure and so, accordingly, we return different negative numbers:
that doesn't mean that every single caller of those functions will
care about the specific failure cause, so the <0 check might still be
perfectly fine even in those cases.
Here we have a small number of named return codes, so I agree with
Dan's approach: comparing by name instead of just checking whether
the return value is negative looks a bit cleaner.
[... 469 lines removed ...]
>
> Reviewed-by: Christophe de Dinechin
Meta: can you please trim the parts of the original message that
you're not specifically replying to? In this particularly egregious
case, less than 10% of the message was actual content rather than
noise.
Please be considerate to list subscribers and make sure they don't
have to waste time and bandwidth fetching additional, unrelated text
just so they can then waste even more time scrolling past them before
getting to your actual reply.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 19/48] remote: in per-driver daemons ensure that state initialize succeeds
Daniel P. Berrangé writes:
> When running in libvirtd, we are happy for any of the drivers to simply
> skip their initialization in virStateInitialize, as other drivers are
> still potentially useful.
>
> When running in per-driver daemons though, we want the daemon to abort
> startup if the driver cannot initialize itself, as the daemon will be
> useless without it.
>
> Reviewed-by: Andrea Bolognani
> Signed-off-by: Daniel P. Berrangé
> ---
> src/bhyve/bhyve_driver.c| 10 +-
> src/driver-state.h | 8 +++-
> src/interface/interface_backend_netcf.c | 8
> src/interface/interface_backend_udev.c | 4 ++--
> src/libvirt.c | 15 ---
> src/libvirt_internal.h | 1 +
> src/libxl/libxl_driver.c| 10 +-
> src/lxc/lxc_driver.c| 12 ++--
> src/network/bridge_driver.c | 4 ++--
> src/node_device/node_device_hal.c | 12 ++--
> src/node_device/node_device_udev.c | 8
> src/nwfilter/nwfilter_driver.c | 12 ++--
> src/qemu/qemu_driver.c | 8
> src/remote/remote_daemon.c | 6 ++
> src/remote/remote_driver.c | 2 +-
> src/secret/secret_driver.c | 8
> src/storage/storage_driver.c| 8
> src/vz/vz_driver.c | 14 +++---
> 18 files changed, 86 insertions(+), 64 deletions(-)
>
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index 5387ac5570..e2c1b00080 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -1220,16 +1220,16 @@ bhyveStateInitialize(bool privileged,
> {
> if (!privileged) {
> VIR_INFO("Not running privileged, disabling driver");
> -return 0;
> +return VIR_DRV_STATE_INIT_SKIPPED;
> }
>
> if (VIR_ALLOC(bhyve_driver) < 0)
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
>
> bhyve_driver->lockFD = -1;
> if (virMutexInit(&bhyve_driver->lock) < 0) {
> VIR_FREE(bhyve_driver);
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
> if (!(bhyve_driver->closeCallbacks = virCloseCallbacksNew()))
> @@ -1303,11 +1303,11 @@ bhyveStateInitialize(bool privileged,
>
> bhyveAutostartDomains(bhyve_driver);
>
> -return 0;
> +return VIR_DRV_STATE_INIT_COMPLETE;
>
> cleanup:
> bhyveStateCleanup();
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
> unsigned
> diff --git a/src/driver-state.h b/src/driver-state.h
> index 974b2252ee..69e2678dfc 100644
> --- a/src/driver-state.h
> +++ b/src/driver-state.h
> @@ -24,7 +24,13 @@
> # error "Don't include this file directly, only use driver.h"
> #endif
>
> -typedef int
> +typedef enum {
> +VIR_DRV_STATE_INIT_ERROR = -1,
> +VIR_DRV_STATE_INIT_SKIPPED,
> +VIR_DRV_STATE_INIT_COMPLETE,
> +} virDrvStateInitResult;
> +
> +typedef virDrvStateInitResult
> (*virDrvStateInitialize)(bool privileged,
> virStateInhibitCallback callback,
> void *opaque);
> diff --git a/src/interface/interface_backend_netcf.c
> b/src/interface/interface_backend_netcf.c
> index 587cee..eb509ccc13 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -93,10 +93,10 @@ netcfStateInitialize(bool privileged,
> void *opaque ATTRIBUTE_UNUSED)
> {
> if (virNetcfDriverStateInitialize() < 0)
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
>
> if (!(driver = virObjectLockableNew(virNetcfDriverStateClass)))
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
>
> driver->privileged = privileged;
>
> @@ -129,12 +129,12 @@ netcfStateInitialize(bool privileged,
> _("failed to initialize netcf"));
> goto error;
> }
> -return 0;
> +return VIR_DRV_STATE_INIT_COMPLETE;
>
> error:
> virObjectUnref(driver);
> driver = NULL;
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
>
> diff --git a/src/interface/interface_backend_udev.c
> b/src/interface/interface_backend_udev.c
> index fea5108dbc..ef748540d1 100644
> --- a/src/interface/interface_backend_udev.c
> +++ b/src/interface/interface_backend_udev.c
> @@ -1172,7 +1172,7 @@ udevStateInitialize(bool privileged,
> virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> void *opaque ATTRIBUTE_UNUSED)
> {
> -int ret = -1;
> +int ret = VIR_DRV_STATE_INIT_ERROR;
>
> if (VIR_ALLOC(driver) < 0)
> goto cleanup;
> @@ -1210,7 +1210,7 @@ udevStateInitialize(bool privileged,
> }
> driver->privileged = privileged;
>
> -ret = 0;
> +ret = VIR_DRV_STATE_INIT_COMPLETE;
>
> cleanup:
> if (ret < 0)
> diff --git a/src/libvirt.c b
[libvirt] [PATCH v3 19/48] remote: in per-driver daemons ensure that state initialize succeeds
When running in libvirtd, we are happy for any of the drivers to simply
skip their initialization in virStateInitialize, as other drivers are
still potentially useful.
When running in per-driver daemons though, we want the daemon to abort
startup if the driver cannot initialize itself, as the daemon will be
useless without it.
Reviewed-by: Andrea Bolognani
Signed-off-by: Daniel P. Berrangé
---
src/bhyve/bhyve_driver.c| 10 +-
src/driver-state.h | 8 +++-
src/interface/interface_backend_netcf.c | 8
src/interface/interface_backend_udev.c | 4 ++--
src/libvirt.c | 15 ---
src/libvirt_internal.h | 1 +
src/libxl/libxl_driver.c| 10 +-
src/lxc/lxc_driver.c| 12 ++--
src/network/bridge_driver.c | 4 ++--
src/node_device/node_device_hal.c | 12 ++--
src/node_device/node_device_udev.c | 8
src/nwfilter/nwfilter_driver.c | 12 ++--
src/qemu/qemu_driver.c | 8
src/remote/remote_daemon.c | 6 ++
src/remote/remote_driver.c | 2 +-
src/secret/secret_driver.c | 8
src/storage/storage_driver.c| 8
src/vz/vz_driver.c | 14 +++---
18 files changed, 86 insertions(+), 64 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 5387ac5570..e2c1b00080 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1220,16 +1220,16 @@ bhyveStateInitialize(bool privileged,
{
if (!privileged) {
VIR_INFO("Not running privileged, disabling driver");
-return 0;
+return VIR_DRV_STATE_INIT_SKIPPED;
}
if (VIR_ALLOC(bhyve_driver) < 0)
-return -1;
+return VIR_DRV_STATE_INIT_ERROR;
bhyve_driver->lockFD = -1;
if (virMutexInit(&bhyve_driver->lock) < 0) {
VIR_FREE(bhyve_driver);
-return -1;
+return VIR_DRV_STATE_INIT_ERROR;
}
if (!(bhyve_driver->closeCallbacks = virCloseCallbacksNew()))
@@ -1303,11 +1303,11 @@ bhyveStateInitialize(bool privileged,
bhyveAutostartDomains(bhyve_driver);
-return 0;
+return VIR_DRV_STATE_INIT_COMPLETE;
cleanup:
bhyveStateCleanup();
-return -1;
+return VIR_DRV_STATE_INIT_ERROR;
}
unsigned
diff --git a/src/driver-state.h b/src/driver-state.h
index 974b2252ee..69e2678dfc 100644
--- a/src/driver-state.h
+++ b/src/driver-state.h
@@ -24,7 +24,13 @@
# error "Don't include this file directly, only use driver.h"
#endif
-typedef int
+typedef enum {
+VIR_DRV_STATE_INIT_ERROR = -1,
+VIR_DRV_STATE_INIT_SKIPPED,
+VIR_DRV_STATE_INIT_COMPLETE,
+} virDrvStateInitResult;
+
+typedef virDrvStateInitResult
(*virDrvStateInitialize)(bool privileged,
virStateInhibitCallback callback,
void *opaque);
diff --git a/src/interface/interface_backend_netcf.c
b/src/interface/interface_backend_netcf.c
index 587cee..eb509ccc13 100644
--- a/src/interface/interface_backend_netcf.c
+++ b/src/interface/interface_backend_netcf.c
@@ -93,10 +93,10 @@ netcfStateInitialize(bool privileged,
void *opaque ATTRIBUTE_UNUSED)
{
if (virNetcfDriverStateInitialize() < 0)
-return -1;
+return VIR_DRV_STATE_INIT_ERROR;
if (!(driver = virObjectLockableNew(virNetcfDriverStateClass)))
-return -1;
+return VIR_DRV_STATE_INIT_ERROR;
driver->privileged = privileged;
@@ -129,12 +129,12 @@ netcfStateInitialize(bool privileged,
_("failed to initialize netcf"));
goto error;
}
-return 0;
+return VIR_DRV_STATE_INIT_COMPLETE;
error:
virObjectUnref(driver);
driver = NULL;
-return -1;
+return VIR_DRV_STATE_INIT_ERROR;
}
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index fea5108dbc..ef748540d1 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -1172,7 +1172,7 @@ udevStateInitialize(bool privileged,
virStateInhibitCallback callback ATTRIBUTE_UNUSED,
void *opaque ATTRIBUTE_UNUSED)
{
-int ret = -1;
+int ret = VIR_DRV_STATE_INIT_ERROR;
if (VIR_ALLOC(driver) < 0)
goto cleanup;
@@ -1210,7 +1210,7 @@ udevStateInitialize(bool privileged,
}
driver->privileged = privileged;
-ret = 0;
+ret = VIR_DRV_STATE_INIT_COMPLETE;
cleanup:
if (ret < 0)
diff --git a/src/libvirt.c b/src/libvirt.c
index f0a768fc7e..9390a767f9 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -629,6 +629,7 @@ virRegisterStateDriver(virStateDriverPtr driver)
/**
* virStateInitialize:
* @privileged: set to true if running with root privilege, false otherwise
+ * @mandatory:
