[PATCH 2/2] virhook: support hooks placed in several files
Michal Privoznik
mprivozn at redhat.com
Mon Jun 22 13:06:38 UTC 2020
On 6/21/20 11:16 PM, Dmitry Nesterenko wrote:
> Signed-off-by: Dmitry Nesterenko <dmitry.nesterenko at 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
More information about the libvir-list
mailing list