[PATCH 0/6] Be tolerant to CH_CMD missing
See 4/6 for more info. The rest is just cleanup. Michal Prívozník (6): ch_conf: Move error reporting into chExtractVersionInfo() chExtractVersionInfo: Don't check for retversion != NULL ch_conf: Dissolve chExtractVersionInfo() in chExtractVersion() ch_driver: Don't error out if CH_CMD was not found chExtractVersion: use g_auto* chExtractVersion: Drop @ret src/ch/ch_conf.c | 53 +++++++++++++++++----------------------------- src/ch/ch_driver.c | 15 +++++++++---- 2 files changed, 30 insertions(+), 38 deletions(-) -- 2.31.1
If chExtractVersionInfo() fails, in some cases it reports error and in some it doesn't. Fix those places and drop reporting error from chExtractVersion() which would just overwrite more specific error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_conf.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index 2dd104b8a8..d900ebc7dd 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -213,11 +213,17 @@ chExtractVersionInfo(int *retversion) tmp = help; /* expected format: cloud-hypervisor v<major>.<minor>.<micro> */ - if ((tmp = STRSKIP(tmp, "cloud-hypervisor v")) == NULL) + if ((tmp = STRSKIP(tmp, "cloud-hypervisor v")) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected output of cloud-hypervisor binary")); goto cleanup; + } - if (virParseVersionString(tmp, &version, true) < 0) + if (virParseVersionString(tmp, &version, true) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse cloud-hypervisor version: %s"), tmp); goto cleanup; + } if (version < MIN_VERSION) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -241,11 +247,8 @@ int chExtractVersion(virCHDriver *driver) if (driver->version > 0) return 0; - if (chExtractVersionInfo(&driver->version) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not extract Cloud-Hypervisor version")); + if (chExtractVersionInfo(&driver->version) < 0) return -1; - } return 0; } -- 2.31.1
The only caller, chExtractVersion() passes not NULL. Therefore, it's redundant to check for NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_conf.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index d900ebc7dd..706e5f0ba4 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -201,8 +201,7 @@ chExtractVersionInfo(int *retversion) g_autofree char *ch_cmd = g_find_program_in_path(CH_CMD); virCommand *cmd = virCommandNewArgList(ch_cmd, "--version", NULL); - if (retversion) - *retversion = 0; + *retversion = 0; virCommandAddEnvString(cmd, "LC_ALL=C"); virCommandSetOutputBuffer(cmd, &help); @@ -231,9 +230,7 @@ chExtractVersionInfo(int *retversion) goto cleanup; } - if (retversion) - *retversion = version; - + *retversion = version; ret = 0; cleanup: -- 2.31.1
After previous patches, there's not much value in chExtractVersion(). Rename chExtractVersionInfo() to chExtractVersion() and have it use virCHDriver directly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_conf.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index 706e5f0ba4..dfebc8525a 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -191,8 +191,8 @@ virCHDriverConfigDispose(void *obj) #define MIN_VERSION ((15 * 1000000) + (0 * 1000) + (0)) -static int -chExtractVersionInfo(int *retversion) +int +chExtractVersion(virCHDriver *driver) { int ret = -1; unsigned long version; @@ -201,8 +201,6 @@ chExtractVersionInfo(int *retversion) g_autofree char *ch_cmd = g_find_program_in_path(CH_CMD); virCommand *cmd = virCommandNewArgList(ch_cmd, "--version", NULL); - *retversion = 0; - virCommandAddEnvString(cmd, "LC_ALL=C"); virCommandSetOutputBuffer(cmd, &help); @@ -230,22 +228,10 @@ chExtractVersionInfo(int *retversion) goto cleanup; } - *retversion = version; + driver->version = version; ret = 0; cleanup: virCommandFree(cmd); - return ret; } - -int chExtractVersion(virCHDriver *driver) -{ - if (driver->version > 0) - return 0; - - if (chExtractVersionInfo(&driver->version) < 0) - return -1; - - return 0; -} -- 2.31.1
The CH driver needs "cloud-hypervisor" binary. And if none was found then the initialization of the driver fails as chStateInitialize() returns VIR_DRV_STATE_INIT_ERROR. This in turn means that whole daemon fails to initialize. Let's return VIR_DRV_STATE_INIT_SKIPPED in this particular case, which disables the CH drvier but lets the daemon run. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_conf.c | 6 +++++- src/ch/ch_driver.c | 15 +++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index dfebc8525a..b2812de7ad 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -199,8 +199,12 @@ chExtractVersion(virCHDriver *driver) char *help = NULL; char *tmp = NULL; g_autofree char *ch_cmd = g_find_program_in_path(CH_CMD); - virCommand *cmd = virCommandNewArgList(ch_cmd, "--version", NULL); + virCommand *cmd = NULL; + if (!ch_cmd) + return -2; + + cmd = virCommandNewArgList(ch_cmd, "--version", NULL); virCommandAddEnvString(cmd, "LC_ALL=C"); virCommandSetOutputBuffer(cmd, &help); diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 7d1e97098a..8c458a20bd 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -836,6 +836,9 @@ static int chStateInitialize(bool privileged, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { + int ret = VIR_DRV_STATE_INIT_ERROR; + int rv; + if (root != NULL) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Driver does not support embedded mode")); @@ -861,14 +864,18 @@ static int chStateInitialize(bool privileged, if (!(ch_driver->config = virCHDriverConfigNew(privileged))) goto cleanup; - if (chExtractVersion(ch_driver) < 0) + if ((rv = chExtractVersion(ch_driver)) < 0) { + if (rv == -2) + ret = VIR_DRV_STATE_INIT_SKIPPED; goto cleanup; + } - return VIR_DRV_STATE_INIT_COMPLETE; + ret = VIR_DRV_STATE_INIT_COMPLETE; cleanup: - chStateCleanup(); - return VIR_DRV_STATE_INIT_ERROR; + if (ret != VIR_DRV_STATE_INIT_COMPLETE) + chStateCleanup(); + return ret; } /* Function Tables */ -- 2.31.1
There are two variables that can be freed automatically: @cmd (which allows us to drop explicit virCommandFree() call at the end of the function) and @help which was never freed (and thus leaked). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index b2812de7ad..c67c815d45 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -196,10 +196,10 @@ chExtractVersion(virCHDriver *driver) { int ret = -1; unsigned long version; - char *help = NULL; + g_autofree char *help = NULL; char *tmp = NULL; g_autofree char *ch_cmd = g_find_program_in_path(CH_CMD); - virCommand *cmd = NULL; + g_autoptr(virCommand) cmd = NULL; if (!ch_cmd) return -2; @@ -236,6 +236,5 @@ chExtractVersion(virCHDriver *driver) ret = 0; cleanup: - virCommandFree(cmd); return ret; } -- 2.31.1
After previous patches, the @ret variable and the 'cleanup' label are redundant. Remove them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_conf.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index c67c815d45..5f156dfe44 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -194,7 +194,6 @@ virCHDriverConfigDispose(void *obj) int chExtractVersion(virCHDriver *driver) { - int ret = -1; unsigned long version; g_autofree char *help = NULL; char *tmp = NULL; @@ -209,7 +208,7 @@ chExtractVersion(virCHDriver *driver) virCommandSetOutputBuffer(cmd, &help); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; tmp = help; @@ -217,24 +216,21 @@ chExtractVersion(virCHDriver *driver) if ((tmp = STRSKIP(tmp, "cloud-hypervisor v")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unexpected output of cloud-hypervisor binary")); - goto cleanup; + return -1; } if (virParseVersionString(tmp, &version, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse cloud-hypervisor version: %s"), tmp); - goto cleanup; + return -1; } if (version < MIN_VERSION) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cloud-Hypervisor version is too old (v15.0 is the minimum supported version)")); - goto cleanup; + return -1; } driver->version = version; - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.31.1
On Fri, Jun 04, 2021 at 03:10:39PM +0200, Michal Privoznik wrote:
See 4/6 for more info. The rest is just cleanup.
Michal Prívozník (6): ch_conf: Move error reporting into chExtractVersionInfo() chExtractVersionInfo: Don't check for retversion != NULL ch_conf: Dissolve chExtractVersionInfo() in chExtractVersion() ch_driver: Don't error out if CH_CMD was not found chExtractVersion: use g_auto* chExtractVersion: Drop @ret
src/ch/ch_conf.c | 53 +++++++++++++++++----------------------------- src/ch/ch_driver.c | 15 +++++++++---- 2 files changed, 30 insertions(+), 38 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> /me facepalms for not actually testing startup with the libvirtd daemon. 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 :|
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik