[PATCH 0/2] Support hooks placed in several files
Libvirt hook calls only one script (/etc/libvirt/hooks/<driver>) now. This is not convenient if scripts for hook are provided by many vendors. Script one vendor can replace previously installed script other vendor. These patches are changing behaviour of hooks to usual linux scheme - running all scripts from directory /etc/libvirt/hooks/<driver>.d in alphabetical order. If we find script in old place we will execute it as first for backward compatibility. Dmitry Nesterenko (2): virthook: refactoring for support hooks placed in several files virhook: support hooks placed in several files src/util/virhook.c | 168 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 146 insertions(+), 22 deletions(-) -- 2.18.4
Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko@virtuozzo.com> --- src/util/virhook.c | 74 +++++++++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 21 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index e499841f66..64ee9a2307 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -221,6 +221,57 @@ virHookPresent(int driver) return 1; } +/** + * virRunScript: + * @path: the script path + * @id: an id for the object '-' if non available for example on daemon hooks + * @op: the operation on the id + * @subop: a sub_operation, currently unused + * @extra: optional string information + * @input: extra input given to the script on stdin + * @output: optional address of variable to store malloced result buffer + * + * Implement a execution of script. This is a synchronous call, we wait for + * execution completion. If @output is non-NULL, *output is guaranteed to be + * allocated after successful virRunScript, and is best-effort allocated after + * failed virRunScript; the caller is responsible for freeing *output. + * + * Returns: 0 if the execution succeeded, -1 if script returned an error + */ +static int +virRunScript(const char *path, + const char *id, + const char *op, + const char *subop, + const char *extra, + const char *input, + char **output) +{ + int ret; + g_autoptr(virCommand) cmd = NULL; + + VIR_DEBUG("Calling hook %s id=%s op=%s subop=%s extra=%s", + path, id, op, subop, extra); + + cmd = virCommandNewArgList(path, id, op, subop, extra, NULL); + + virCommandAddEnvPassCommon(cmd); + + if (input) + virCommandSetInputBuffer(cmd, input); + if (output) + virCommandSetOutputBuffer(cmd, output); + + ret = virCommandRun(cmd, NULL); + if (ret < 0) { + /* Convert INTERNAL_ERROR into known error. */ + virReportError(VIR_ERR_HOOK_SCRIPT_FAILED, "%s", + virGetLastErrorMessage()); + } + + return ret; +} + /** * virHookCall: * @driver: the driver number (from virHookDriver enum) @@ -249,7 +300,6 @@ virHookCall(int driver, const char *input, char **output) { - int ret; g_autofree char *path = NULL; g_autoptr(virCommand) cmd = NULL; const char *drvstr; @@ -318,24 +368,6 @@ virHookCall(int driver, return -1; } - VIR_DEBUG("Calling hook opstr=%s subopstr=%s extra=%s", - opstr, subopstr, extra); - - cmd = virCommandNewArgList(path, id, opstr, subopstr, extra, NULL); - - virCommandAddEnvPassCommon(cmd); - - if (input) - virCommandSetInputBuffer(cmd, input); - if (output) - virCommandSetOutputBuffer(cmd, output); - - ret = virCommandRun(cmd, NULL); - if (ret < 0) { - /* Convert INTERNAL_ERROR into known error. */ - virReportError(VIR_ERR_HOOK_SCRIPT_FAILED, "%s", - virGetLastErrorMessage()); - } - - return ret; + return virRunScript(path, id, opstr, subopstr, extra, + input, output); } -- 2.18.4
On 6/21/20 11:16 PM, Dmitry Nesterenko wrote:
Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko@virtuozzo.com> --- src/util/virhook.c | 74 +++++++++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 21 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko@virtuozzo.com> --- src/util/virhook.c | 110 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 9 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index 64ee9a2307..763381d962 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -33,6 +33,7 @@ #include "virfile.h" #include "configmake.h" #include "vircommand.h" +#include "virstring.h" #define VIR_FROM_THIS VIR_FROM_HOOK @@ -141,7 +142,11 @@ static int virHooksFound = -1; static int virHookCheck(int no, const char *driver) { + int ret; + DIR *dir; + struct dirent *entry; g_autofree char *path = NULL; + g_autofree char *dir_path = NULL; if (driver == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -158,16 +163,39 @@ virHookCheck(int no, const char *driver) if (!virFileExists(path)) { VIR_DEBUG("No hook script %s", path); - return 0; + } else if (!virFileIsExecutable(path)) { + VIR_WARN("Non-executable hook script %s", path); + } else { + VIR_DEBUG("Found hook script %s", path); + return 1; } - if (!virFileIsExecutable(path)) { - VIR_WARN("Non-executable hook script %s", path); + dir_path = g_strdup_printf("%s.d", path); + if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + return -1; + + if (!ret) { + VIR_DEBUG("No hook script dir %s", dir_path); return 0; } - VIR_DEBUG("Found hook script %s", path); - return 1; + while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { + g_autofree char *entry_path = g_build_filename(dir_path, + entry->d_name, + NULL); + if (!virFileIsExecutable(entry_path)) { + VIR_WARN("Non-executable hook script %s", entry_path); + continue; + } + + VIR_DEBUG("Found hook script %s", entry_path); + ret = 1; + break; + } + + VIR_DIR_CLOSE(dir); + + return ret; } /* @@ -282,7 +310,7 @@ virRunScript(const char *path, * @input: extra input given to the script on stdin * @output: optional address of variable to store malloced result buffer * - * Implement a hook call, where the external script for the driver is + * Implement a hook call, where the external scripts for the driver is * called with the given information. This is a synchronous call, we wait for * execution completion. If @output is non-NULL, *output is guaranteed to be * allocated after successful virHookCall, and is best-effort allocated after @@ -300,11 +328,16 @@ virHookCall(int driver, const char *input, char **output) { + int ret, script_ret; + DIR *dir; + struct dirent *entry; g_autofree char *path = NULL; - g_autoptr(virCommand) cmd = NULL; + g_autofree char *dir_path = NULL; + VIR_AUTOSTRINGLIST entries = NULL; const char *drvstr; const char *opstr; const char *subopstr; + size_t i, nentries; if (output) *output = NULL; @@ -368,6 +401,65 @@ virHookCall(int driver, return -1; } - return virRunScript(path, id, opstr, subopstr, extra, - input, output); + script_ret = 1; + + if (virFileExists(path) && virFileIsExecutable(path)) { + script_ret = virRunScript(path, id, opstr, subopstr, extra, + input, output); + if (script_ret < 0 && output) + return -1; + } + + dir_path = g_strdup_printf("%s.d", path); + if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + return -1; + + if (!ret) + return script_ret; + + while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { + g_autofree char *entry_path = g_build_filename(dir_path, + entry->d_name, + NULL); + if (!virFileIsExecutable(entry_path)) + continue; + + virStringListAdd(&entries, entry_path); + } + + VIR_DIR_CLOSE(dir); + + if (ret < 0) + return -1; + + if (!entries) + return script_ret; + + nentries = virStringListLength((const char **)entries); + qsort(entries, nentries, sizeof(*entries), virStringSortCompare); + + for (i = 0; i < nentries; i++) { + int entry_ret; + char *entry_input; + g_autofree char *entry_output = NULL; + + /* Get input from previous output */ + entry_input = (!script_ret && output && + !virStringIsEmpty(*output)) ? *output : (char *)input; + entry_ret = virRunScript(entries[i], id, opstr, + subopstr, extra, entry_input, + (output) ? &entry_output : NULL); + if (entry_ret < 0 && output) + return -1; + if (entry_ret < script_ret) + script_ret = entry_ret; + + /* Replace output to new output from item */ + if (output && !virStringIsEmpty(entry_output)) { + g_free(*output); + *output = g_steal_pointer(&entry_output); + } + } + + return script_ret; } -- 2.18.4
On 6/21/20 11:16 PM, Dmitry Nesterenko wrote:
Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko@virtuozzo.com> --- src/util/virhook.c | 110 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 9 deletions(-)
diff --git a/src/util/virhook.c b/src/util/virhook.c index 64ee9a2307..763381d962 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -33,6 +33,7 @@ #include "virfile.h" #include "configmake.h" #include "vircommand.h" +#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_HOOK
@@ -141,7 +142,11 @@ static int virHooksFound = -1; static int virHookCheck(int no, const char *driver) { + int ret; + DIR *dir; + struct dirent *entry; g_autofree char *path = NULL; + g_autofree char *dir_path = NULL;
if (driver == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -158,16 +163,39 @@ virHookCheck(int no, const char *driver)
if (!virFileExists(path)) { VIR_DEBUG("No hook script %s", path); - return 0; + } else if (!virFileIsExecutable(path)) { + VIR_WARN("Non-executable hook script %s", path); + } else { + VIR_DEBUG("Found hook script %s", path); + return 1; }
- if (!virFileIsExecutable(path)) { - VIR_WARN("Non-executable hook script %s", path); + dir_path = g_strdup_printf("%s.d", path); + if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + return -1; + + if (!ret) { + VIR_DEBUG("No hook script dir %s", dir_path); return 0; }
- VIR_DEBUG("Found hook script %s", path); - return 1; + while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { + g_autofree char *entry_path = g_build_filename(dir_path, + entry->d_name, + NULL); + if (!virFileIsExecutable(entry_path)) { + VIR_WARN("Non-executable hook script %s", entry_path); + continue; + } + + VIR_DEBUG("Found hook script %s", entry_path); + ret = 1; + break; + } + + VIR_DIR_CLOSE(dir); + + return ret; }
/* @@ -282,7 +310,7 @@ virRunScript(const char *path, * @input: extra input given to the script on stdin * @output: optional address of variable to store malloced result buffer * - * Implement a hook call, where the external script for the driver is + * Implement a hook call, where the external scripts for the driver is * called with the given information. This is a synchronous call, we wait for * execution completion. If @output is non-NULL, *output is guaranteed to be * allocated after successful virHookCall, and is best-effort allocated after @@ -300,11 +328,16 @@ virHookCall(int driver, const char *input, char **output) { + int ret, script_ret; + DIR *dir; + struct dirent *entry; g_autofree char *path = NULL; - g_autoptr(virCommand) cmd = NULL; + g_autofree char *dir_path = NULL; + VIR_AUTOSTRINGLIST entries = NULL; const char *drvstr; const char *opstr; const char *subopstr; + size_t i, nentries;
if (output) *output = NULL; @@ -368,6 +401,65 @@ virHookCall(int driver, return -1; }
- return virRunScript(path, id, opstr, subopstr, extra, - input, output); + script_ret = 1; + + if (virFileExists(path) && virFileIsExecutable(path)) {
virFileIsExecutable() is enough. It won't return true for a non-existent file.
+ script_ret = virRunScript(path, id, opstr, subopstr, extra, + input, output); + if (script_ret < 0 && output)
I'm not sure we want this check for output. What if caller passed output = NULL - just like can be seen in the hunk above? But this opens a good question - whether to stop processing hook scripts on the first failure or continue through all scripts and report error only if at least one of them returned failure. I think we should go with the former and stop processing at the first failure.
+ return -1; + } + + dir_path = g_strdup_printf("%s.d", path); + if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + return -1; + + if (!ret) + return script_ret; + + while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { + g_autofree char *entry_path = g_build_filename(dir_path, + entry->d_name, + NULL); + if (!virFileIsExecutable(entry_path)) + continue; + + virStringListAdd(&entries, entry_path); + } + + VIR_DIR_CLOSE(dir); + + if (ret < 0) + return -1; + + if (!entries) + return script_ret; + + nentries = virStringListLength((const char **)entries); + qsort(entries, nentries, sizeof(*entries), virStringSortCompare); + + for (i = 0; i < nentries; i++) { + int entry_ret; + char *entry_input; + g_autofree char *entry_output = NULL; + + /* Get input from previous output */ + entry_input = (!script_ret && output && + !virStringIsEmpty(*output)) ? *output : (char *)input; + entry_ret = virRunScript(entries[i], id, opstr, + subopstr, extra, entry_input, + (output) ? &entry_output : NULL); + if (entry_ret < 0 && output) + return -1; + if (entry_ret < script_ret) + script_ret = entry_ret; + + /* Replace output to new output from item */ + if (output && !virStringIsEmpty(entry_output)) { + g_free(*output); + *output = g_steal_pointer(&entry_output); + }
This is not something we've discussed ;-) But I guess it only makes sense. If you have X hook scripts, each one modifying only the part of the domain XML then you want to chain their outputs to produce the desired output. Do we want to feed the first script here with the output of the old one? I mean, if you have both: /etc/libvirt/hooks/qemu /etc/libvirt/hooks/qemu.d/01-hook /etc/libvirt/hooks/qemu.d/02-hook they will be executed in this order. Good. And 01-hook will pass the output to 02-hook. Good. But what about 'qemu' and 01-hook? It's only fair to chain the output, isn't it? Anyway, this is what I'd squash in: diff --git i/src/util/virhook.c w/src/util/virhook.c index 763381d962..f0c44d5c9d 100644 --- i/src/util/virhook.c +++ w/src/util/virhook.c @@ -328,8 +328,8 @@ virHookCall(int driver, const char *input, char **output) { - int ret, script_ret; - DIR *dir; + int ret; + DIR *dir = NULL; struct dirent *entry; g_autofree char *path = NULL; g_autofree char *dir_path = NULL; @@ -337,6 +337,7 @@ virHookCall(int driver, const char *drvstr; const char *opstr; const char *subopstr; + const char *entry_input = input; size_t i, nentries; if (output) @@ -401,21 +402,21 @@ virHookCall(int driver, return -1; } - script_ret = 1; - - if (virFileExists(path) && virFileIsExecutable(path)) { - script_ret = virRunScript(path, id, opstr, subopstr, extra, - input, output); - if (script_ret < 0 && output) + if (virFileIsExecutable(path)) { + if (virRunScript(path, id, opstr, subopstr, extra, entry_input, output) < 0) return -1; + + /* The script output is input of next script. */ + if (output) + entry_input = *output; } dir_path = g_strdup_printf("%s.d", path); - if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + if (virDirOpenIfExists(&dir, dir_path) < 0) return -1; - if (!ret) - return script_ret; + if (!dir) + return 0; while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { g_autofree char *entry_path = g_build_filename(dir_path, @@ -433,33 +434,26 @@ virHookCall(int driver, return -1; if (!entries) - return script_ret; + return 0; nentries = virStringListLength((const char **)entries); qsort(entries, nentries, sizeof(*entries), virStringSortCompare); for (i = 0; i < nentries; i++) { - int entry_ret; - char *entry_input; - g_autofree char *entry_output = NULL; + char *entry_output = NULL; - /* Get input from previous output */ - entry_input = (!script_ret && output && - !virStringIsEmpty(*output)) ? *output : (char *)input; - entry_ret = virRunScript(entries[i], id, opstr, - subopstr, extra, entry_input, - (output) ? &entry_output : NULL); - if (entry_ret < 0 && output) + if (virRunScript(entries[i], id, opstr, + subopstr, extra, entry_input, + (output) ? &entry_output : NULL) < 0) return -1; - if (entry_ret < script_ret) - script_ret = entry_ret; /* Replace output to new output from item */ - if (output && !virStringIsEmpty(entry_output)) { + if (output) { g_free(*output); *output = g_steal_pointer(&entry_output); + entry_input = *output; } } - return script_ret; + return 0; } Michal
virFileIsExecutable() is enough
I agree and will fix it in next version of patch.
I'm not sure we want this check for output
If we are given param for xml output - it is call hook for "migrate" or "restore" and any fail from script can break the process. So we can break running of all scripts immediately after first fail. But if we don't have xml param for output - it can be call hook for stop with ignoring return code of script. And I think in this case will better to run all scripts.
But what about 'qemu' and 01-hook? It's only fair to chain the output, isn't it?
Yes of course. And I did it. Output from first script will be used as input for next script. See next lines: /* Get input from previous output */ entry_input = (!script_ret && output && !virStringIsEmpty(*output)) ? *output : (char *)input; ________________________________________ From: Michal Privoznik <mprivozn@redhat.com> Sent: Monday, June 22, 2020 4:06 PM To: Dmitry Nesterenko; libvir-list@redhat.com Subject: Re: [PATCH 2/2] virhook: support hooks placed in several files On 6/21/20 11:16 PM, Dmitry Nesterenko wrote:
Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko@virtuozzo.com> --- src/util/virhook.c | 110 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 9 deletions(-)
diff --git a/src/util/virhook.c b/src/util/virhook.c index 64ee9a2307..763381d962 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -33,6 +33,7 @@ #include "virfile.h" #include "configmake.h" #include "vircommand.h" +#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_HOOK
@@ -141,7 +142,11 @@ static int virHooksFound = -1; static int virHookCheck(int no, const char *driver) { + int ret; + DIR *dir; + struct dirent *entry; g_autofree char *path = NULL; + g_autofree char *dir_path = NULL;
if (driver == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -158,16 +163,39 @@ virHookCheck(int no, const char *driver)
if (!virFileExists(path)) { VIR_DEBUG("No hook script %s", path); - return 0; + } else if (!virFileIsExecutable(path)) { + VIR_WARN("Non-executable hook script %s", path); + } else { + VIR_DEBUG("Found hook script %s", path); + return 1; }
- if (!virFileIsExecutable(path)) { - VIR_WARN("Non-executable hook script %s", path); + dir_path = g_strdup_printf("%s.d", path); + if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + return -1; + + if (!ret) { + VIR_DEBUG("No hook script dir %s", dir_path); return 0; }
- VIR_DEBUG("Found hook script %s", path); - return 1; + while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { + g_autofree char *entry_path = g_build_filename(dir_path, + entry->d_name, + NULL); + if (!virFileIsExecutable(entry_path)) { + VIR_WARN("Non-executable hook script %s", entry_path); + continue; + } + + VIR_DEBUG("Found hook script %s", entry_path); + ret = 1; + break; + } + + VIR_DIR_CLOSE(dir); + + return ret; }
/* @@ -282,7 +310,7 @@ virRunScript(const char *path, * @input: extra input given to the script on stdin * @output: optional address of variable to store malloced result buffer * - * Implement a hook call, where the external script for the driver is + * Implement a hook call, where the external scripts for the driver is * called with the given information. This is a synchronous call, we wait for * execution completion. If @output is non-NULL, *output is guaranteed to be * allocated after successful virHookCall, and is best-effort allocated after @@ -300,11 +328,16 @@ virHookCall(int driver, const char *input, char **output) { + int ret, script_ret; + DIR *dir; + struct dirent *entry; g_autofree char *path = NULL; - g_autoptr(virCommand) cmd = NULL; + g_autofree char *dir_path = NULL; + VIR_AUTOSTRINGLIST entries = NULL; const char *drvstr; const char *opstr; const char *subopstr; + size_t i, nentries;
if (output) *output = NULL; @@ -368,6 +401,65 @@ virHookCall(int driver, return -1; }
- return virRunScript(path, id, opstr, subopstr, extra, - input, output); + script_ret = 1; + + if (virFileExists(path) && virFileIsExecutable(path)) {
virFileIsExecutable() is enough. It won't return true for a non-existent file.
+ script_ret = virRunScript(path, id, opstr, subopstr, extra, + input, output); + if (script_ret < 0 && output)
I'm not sure we want this check for output. What if caller passed output = NULL - just like can be seen in the hunk above? But this opens a good question - whether to stop processing hook scripts on the first failure or continue through all scripts and report error only if at least one of them returned failure. I think we should go with the former and stop processing at the first failure.
+ return -1; + } + + dir_path = g_strdup_printf("%s.d", path); + if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + return -1; + + if (!ret) + return script_ret; + + while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { + g_autofree char *entry_path = g_build_filename(dir_path, + entry->d_name, + NULL); + if (!virFileIsExecutable(entry_path)) + continue; + + virStringListAdd(&entries, entry_path); + } + + VIR_DIR_CLOSE(dir); + + if (ret < 0) + return -1; + + if (!entries) + return script_ret; + + nentries = virStringListLength((const char **)entries); + qsort(entries, nentries, sizeof(*entries), virStringSortCompare); + + for (i = 0; i < nentries; i++) { + int entry_ret; + char *entry_input; + g_autofree char *entry_output = NULL; + + /* Get input from previous output */ + entry_input = (!script_ret && output && + !virStringIsEmpty(*output)) ? *output : (char *)input; + entry_ret = virRunScript(entries[i], id, opstr, + subopstr, extra, entry_input, + (output) ? &entry_output : NULL); + if (entry_ret < 0 && output) + return -1; + if (entry_ret < script_ret) + script_ret = entry_ret; + + /* Replace output to new output from item */ + if (output && !virStringIsEmpty(entry_output)) { + g_free(*output); + *output = g_steal_pointer(&entry_output); + }
This is not something we've discussed ;-) But I guess it only makes sense. If you have X hook scripts, each one modifying only the part of the domain XML then you want to chain their outputs to produce the desired output. Do we want to feed the first script here with the output of the old one? I mean, if you have both: /etc/libvirt/hooks/qemu /etc/libvirt/hooks/qemu.d/01-hook /etc/libvirt/hooks/qemu.d/02-hook they will be executed in this order. Good. And 01-hook will pass the output to 02-hook. Good. But what about 'qemu' and 01-hook? It's only fair to chain the output, isn't it? Anyway, this is what I'd squash in: diff --git i/src/util/virhook.c w/src/util/virhook.c index 763381d962..f0c44d5c9d 100644 --- i/src/util/virhook.c +++ w/src/util/virhook.c @@ -328,8 +328,8 @@ virHookCall(int driver, const char *input, char **output) { - int ret, script_ret; - DIR *dir; + int ret; + DIR *dir = NULL; struct dirent *entry; g_autofree char *path = NULL; g_autofree char *dir_path = NULL; @@ -337,6 +337,7 @@ virHookCall(int driver, const char *drvstr; const char *opstr; const char *subopstr; + const char *entry_input = input; size_t i, nentries; if (output) @@ -401,21 +402,21 @@ virHookCall(int driver, return -1; } - script_ret = 1; - - if (virFileExists(path) && virFileIsExecutable(path)) { - script_ret = virRunScript(path, id, opstr, subopstr, extra, - input, output); - if (script_ret < 0 && output) + if (virFileIsExecutable(path)) { + if (virRunScript(path, id, opstr, subopstr, extra, entry_input, output) < 0) return -1; + + /* The script output is input of next script. */ + if (output) + entry_input = *output; } dir_path = g_strdup_printf("%s.d", path); - if ((ret = virDirOpenIfExists(&dir, dir_path)) < 0) + if (virDirOpenIfExists(&dir, dir_path) < 0) return -1; - if (!ret) - return script_ret; + if (!dir) + return 0; while ((ret = virDirRead(dir, &entry, dir_path)) > 0) { g_autofree char *entry_path = g_build_filename(dir_path, @@ -433,33 +434,26 @@ virHookCall(int driver, return -1; if (!entries) - return script_ret; + return 0; nentries = virStringListLength((const char **)entries); qsort(entries, nentries, sizeof(*entries), virStringSortCompare); for (i = 0; i < nentries; i++) { - int entry_ret; - char *entry_input; - g_autofree char *entry_output = NULL; + char *entry_output = NULL; - /* Get input from previous output */ - entry_input = (!script_ret && output && - !virStringIsEmpty(*output)) ? *output : (char *)input; - entry_ret = virRunScript(entries[i], id, opstr, - subopstr, extra, entry_input, - (output) ? &entry_output : NULL); - if (entry_ret < 0 && output) + if (virRunScript(entries[i], id, opstr, + subopstr, extra, entry_input, + (output) ? &entry_output : NULL) < 0) return -1; - if (entry_ret < script_ret) - script_ret = entry_ret; /* Replace output to new output from item */ - if (output && !virStringIsEmpty(entry_output)) { + if (output) { g_free(*output); *output = g_steal_pointer(&entry_output); + entry_input = *output; } } - return script_ret; + return 0; } Michal
On 6/23/20 10:24 AM, Dmitry Nesterenko wrote:
virFileIsExecutable() is enough
I agree and will fix it in next version of patch.
I'm not sure we want this check for output
If we are given param for xml output - it is call hook for "migrate" or "restore" and any fail from script can break the process. So we can break running of all scripts immediately after first fail. But if we don't have xml param for output - it can be call hook for stop with ignoring return code of script. And I think in this case will better to run all scripts.
Well, apparently output != NULL is not enough to tell whether we can stop or not. For instance: qemuProcessStartHook() which is called when a domain is starting up. The output is NULL (the script can't change the domain XML), but if it fails the start of the domain is aborted. But that leads me to think that maybe after all we should run all hook scripts and report failure if either of them failed. Because of instance, if I have two hook scripts, one sets up one resource for the domain, the other sets up some other resource for the domain; and both of them would be run on domain startup I want them both to run on domain shutdown to release the resources. But, if say the first script fails on domain startup, so the startup process is aborted, qemuProcessStop() is called which executes both scripts again. I mean, we would not have paired "prepare" and "release" calls for both scripts. We can declare that hook scripts have to take care of that and to some extend they already are doing so, because even now, with one hook script per driver the "release" implementation must cope with unbalanced call, because if domain start up fails slightly earlier, before "prepare" is called, then "release" is called anyway. Michal
But that leads me to think that maybe after all we should run all hook scripts and report failure if either of them failed. we would not have paired "prepare" and "release" calls for both scripts.
I agree with you. I will do this scheme - always run all scripts. Dmitry ________________________________________ From: Michal Privoznik <mprivozn@redhat.com> Sent: Tuesday, June 23, 2020 1:03 PM To: Dmitry Nesterenko; libvir-list@redhat.com Subject: Re: [PATCH 2/2] virhook: support hooks placed in several files On 6/23/20 10:24 AM, Dmitry Nesterenko wrote:
virFileIsExecutable() is enough
I agree and will fix it in next version of patch.
I'm not sure we want this check for output
If we are given param for xml output - it is call hook for "migrate" or "restore" and any fail from script can break the process. So we can break running of all scripts immediately after first fail. But if we don't have xml param for output - it can be call hook for stop with ignoring return code of script. And I think in this case will better to run all scripts.
Well, apparently output != NULL is not enough to tell whether we can stop or not. For instance: qemuProcessStartHook() which is called when a domain is starting up. The output is NULL (the script can't change the domain XML), but if it fails the start of the domain is aborted. But that leads me to think that maybe after all we should run all hook scripts and report failure if either of them failed. Because of instance, if I have two hook scripts, one sets up one resource for the domain, the other sets up some other resource for the domain; and both of them would be run on domain startup I want them both to run on domain shutdown to release the resources. But, if say the first script fails on domain startup, so the startup process is aborted, qemuProcessStop() is called which executes both scripts again. I mean, we would not have paired "prepare" and "release" calls for both scripts. We can declare that hook scripts have to take care of that and to some extend they already are doing so, because even now, with one hook script per driver the "release" implementation must cope with unbalanced call, because if domain start up fails slightly earlier, before "prepare" is called, then "release" is called anyway. Michal
On 6/21/20 11:16 PM, Dmitry Nesterenko wrote:
Libvirt hook calls only one script (/etc/libvirt/hooks/<driver>) now. This is not convenient if scripts for hook are provided by many vendors. Script one vendor can replace previously installed script other vendor. These patches are changing behaviour of hooks to usual linux scheme - running all scripts from directory /etc/libvirt/hooks/<driver>.d in alphabetical order. If we find script in old place we will execute it as first for backward compatibility.
Dmitry Nesterenko (2): virthook: refactoring for support hooks placed in several files virhook: support hooks placed in several files
src/util/virhook.c | 168 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 146 insertions(+), 22 deletions(-)
What I am missing here is: - documentation (we have docs/hooks.html.in documenting how the hook scripts are handled) - news.xml - this is user visible change that is possibly interesting to them Otherwise looking good. Michal
participants (2)
-
Dmitry Nesterenko -
Michal Privoznik