[PATCH 0 of 3] (#2) [RFC] Add external migration checks

Changes to reflect change to base VSMigrationService MSD change

# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205184040 25200 # Node ID c097aab08ff886ac32a6f4a09b05c7880570704a # Parent 991f50b2c972bf02ab61f5696c25f5194bc4d640 [RFC] Add optional check parameter field to migration schema Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 991f50b2c972 -r c097aab08ff8 schema/VSMigrationSettingData.mof --- a/schema/VSMigrationSettingData.mof Mon Mar 10 14:20:38 2008 -0700 +++ b/schema/VSMigrationSettingData.mof Mon Mar 10 14:20:40 2008 -0700 @@ -11,6 +11,7 @@ class Xen_VirtualSystemMigrationSettingD [ ValueMap {"0","1","2","3","4","5","6"}, Values { "Unknown", "Other", "SSH", "TLS", "TLS Strict", "TCP", "UNIX" }] uint16 TransportType; + string CheckParameters[]; }; [Provider("cmpi::Virt_VSMigrationSettingData")] @@ -18,4 +19,5 @@ class KVM_VirtualSystemMigrationSettingD [ ValueMap {"0","1","2","3","4","5","6"}, Values { "Unknown", "Other", "SSH", "TLS", "TLS Strict", "TCP", "UNIX" }] uint16 TransportType; + string CheckParameters[]; };

# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205184041 25200 # Node ID 8eb16f25ec12565c54e553972299fdbdda12533f # Parent c097aab08ff886ac32a6f4a09b05c7880570704a [RFC] Add configure support for migration check settings Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r c097aab08ff8 -r 8eb16f25ec12 configure.ac --- a/configure.ac Mon Mar 10 14:20:40 2008 -0700 +++ b/configure.ac Mon Mar 10 14:20:41 2008 -0700 @@ -72,6 +72,20 @@ AC_ARG_WITH(html-dir, [test "x$withval" != "x" && HTML_DIR="$withval"], [HTML_DIR='$(datadir)/doc/$(PACKAGE)-$(VERSION)/html']) AC_SUBST(HTML_DIR) + +AC_ARG_WITH([migrate_check_timeout], + [ --with-migrate_check_timeout=SECS Max runtime allowed for external migration checks], + [test "x$withval" != "x" && MIG_CHECKS_TIMEOUT="$withval"], + [MIG_CHECKS_TIMEOUT=10]) +AC_SUBST(MIG_CHECKS_TIMEOUT) +AC_DEFINE_UNQUOTED(MIG_CHECKS_TIMEOUT, $MIG_CHECKS_TIMEOUT, [External migration check timeout]) + +AC_ARG_WITH([migrate_check_dir], + [ --with-migrate_check_dir=dir Location of external migration checks], + [test "x$withval" != "x" && MIG_CHECKS_DIR="$withval"], + [MIG_CHECKS_DIR=/usr/libexec/extchecks]) +AC_SUBST(MIG_CHECKS_DIR) +AC_DEFINE_UNQUOTED(MIG_CHECKS_DIR, "$MIG_CHECKS_DIR", [External migration check timeout]) # Autogenerate the autoconf header file to store build settings AC_CONFIG_HEADER([config.h])

# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205184074 25200 # Node ID a9134d48bc71c419c78b7e58a71f873281fe84ac # Parent 8eb16f25ec12565c54e553972299fdbdda12533f [RFC] Add external check functionality to CheckIsVSMigratable() Changes: - Fetch the array out of the MSD ourselves to avoid adding a special case to get_msd_values() Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 8eb16f25ec12 -r a9134d48bc71 src/Virt_VSMigrationService.c --- a/src/Virt_VSMigrationService.c Mon Mar 10 14:20:41 2008 -0700 +++ b/src/Virt_VSMigrationService.c Mon Mar 10 14:21:14 2008 -0700 @@ -21,6 +21,13 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <sys/stat.h> +#include <signal.h> +#include <unistd.h> +#include <dirent.h> +#include <errno.h> #include <uuid/uuid.h> @@ -39,6 +46,8 @@ #include "Virt_VSMigrationService.h" #include "Virt_HostSystem.h" #include "Virt_VSMigrationSettingData.h" + +#include "config.h" #define CIM_JOBSTATE_STARTING 3 #define CIM_JOBSTATE_RUNNING 4 @@ -210,7 +219,6 @@ static CMPIStatus get_msd_values(const C "Failed to connect to remote host (%s)", uri); goto out; } - out: free(uri); @@ -260,6 +268,258 @@ static CMPIStatus check_hver(virConnectP ""); out: return s; +} + +static bool is_valid_check(const char *path) +{ + struct stat s; + + if (stat(path, &s) != 0) + return false; + + if (!S_ISREG(s.st_mode)) + return false; + + if ((s.st_mode & S_IXUSR) || + (s.st_mode & S_IXGRP) || + (s.st_mode & S_IXOTH)) + return true; + else + return false; +} + +static void free_list(char **list, int count) +{ + int i; + + if (list == NULL) + return; + + for (i = 0; i < count; i++) + free(list[i]); + + free(list); +} + +static char **list_migration_checks(int *count) +{ + DIR *dir; + struct dirent *de; + char **list = NULL; + int len = 0; + + *count = 0; + + dir = opendir(MIG_CHECKS_DIR); + if (dir == NULL) { + CU_DEBUG("Unable to open migration checks dir: %s (%s)", + MIG_CHECKS_DIR, + strerror(errno)); + *count = -1; + return NULL; + } + + while ((de = readdir(dir)) != NULL) { + int ret; + char *path = NULL; + + if (de->d_name[0] == '.') + continue; + + if (*count == len) { + char **tmp; + + len = (len * 2) + 1; + tmp = realloc(list, sizeof(char *) * len); + if (tmp == NULL) { + CU_DEBUG("Failed to alloc check list"); + goto error; + } + + list = tmp; + } + + ret = asprintf(&path, + "%s/%s", + MIG_CHECKS_DIR, + de->d_name); + if (ret == -1) { + CU_DEBUG("Failed to alloc path for check"); + goto error; + } + + if (is_valid_check(path)) { + list[*count] = path; + (*count) += 1; + } else { + CU_DEBUG("Invalid check program: %s", path); + free(path); + } + } + + closedir(dir); + + return list; + error: + closedir(dir); + + free_list(list, *count); + *count = 0; + + return NULL; +} + +static CMPIStatus _call_check(virDomainPtr dom, + const char *prog, + const char *param_path) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + pid_t pid; + int i; + int rc = -1; + + pid = fork(); + if (pid == 0) { + virConnectPtr conn = virDomainGetConnect(dom); + const char *name = virDomainGetName(dom); + const char *uri = virConnectGetURI(conn); + + if (setpgrp() == -1) + perror("setpgrp"); + + execl(prog, prog, name, uri, param_path, NULL); + CU_DEBUG("exec(%s) failed: %s", prog, strerror(errno)); + _exit(1); + } + + for (i = 0; i < (MIG_CHECKS_TIMEOUT * 4); i++) { + int status; + if (waitpid(pid, &status, WNOHANG) != pid) { + usleep(250000); + } else { + rc = WEXITSTATUS(status); + break; + } + } + + if (rc == -1) { + CU_DEBUG("Killing off stale child %i", pid); + killpg(pid, SIGKILL); + waitpid(pid, NULL, WNOHANG); + } + + if (rc != 0) { + char *name = strdup(prog); + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration check `%s' failed", + basename(name)); + free(name); + } + + return s; +} + +static CMPIStatus call_external_checks(virDomainPtr dom, + const char *param_path) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + char **list = NULL; + int count = 0; + int i; + + list = list_migration_checks(&count); + if (count < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to execute migration checks"); + goto out; + } else if (list == NULL) { + goto out; + } + + for (i = 0; i < count; i++) { + CU_DEBUG("Calling migration check: %s", list[i]); + s = _call_check(dom, list[i], param_path); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("...Failed"); + break; + } else + CU_DEBUG("...OK"); + } + out: + free_list(list, count); + + return s; +} + +static char *write_params(CMPIArray *array) +{ + int i; + int fd; + char *filename = strdup("/tmp/libvirtcim_mig.XXXXXX"); + FILE *file = NULL; + + if (filename == NULL) { + CU_DEBUG("Unable to get temporary file"); + return NULL; + } + + fd = mkstemp(filename); + if (fd < 0) { + CU_DEBUG("Unable to get temporary file: %s", strerror(errno)); + free(filename); + filename = NULL; + goto out; + } + + file = fdopen(fd, "w"); + if (file == NULL) { + CU_DEBUG("Unable to open temporary file: %s", strerror(errno)); + free(filename); + filename = NULL; + goto out; + } + + for (i = 0; i < CMGetArrayCount(array, NULL); i++) { + CMPIData d; + CMPIStatus s; + + d = CMGetArrayElementAt(array, i, &s); + if ((s.rc != CMPI_RC_OK) || CMIsNullValue(d)) { + CU_DEBUG("Unable to get array[%i]: %s", + i, + CMGetCharPtr(s.msg)); + continue; + } + + fprintf(file, "%s\n", CMGetCharPtr(d.value.string)); + } + + out: + if (file != NULL) + fclose(file); + + close(fd); + + return filename; +} + +static char *get_parms_file(const CMPIObjectPath *ref, + const CMPIArgs *argsin) +{ + CMPIStatus s; + CMPIArray *array; + CMPIInstance *msd; + + s = get_msd(ref, argsin, &msd); + if (s.rc != CMPI_RC_OK) + return NULL; + + if (cu_get_array_prop(msd, "CheckParameters", &array) == CMPI_RC_OK) + return write_params(array); + else + return NULL; } static CMPIStatus vs_migratable(const CMPIObjectPath *ref, @@ -276,6 +536,7 @@ static CMPIStatus vs_migratable(const CM CMPIBoolean isMigratable = 0; uint16_t type; virDomainPtr dom = NULL; + char *path = NULL; s = get_msd_values(ref, destination, argsin, &type, &dconn); if (s.rc != CMPI_RC_OK) @@ -298,6 +559,11 @@ static CMPIStatus vs_migratable(const CM } s = check_caps(conn, dconn); + if (s.rc != CMPI_RC_OK) + goto out; + + path = get_parms_file(ref, argsin); + s = call_external_checks(dom, path); if (s.rc != CMPI_RC_OK) goto out; @@ -315,6 +581,10 @@ static CMPIStatus vs_migratable(const CM virDomainFree(dom); virConnectClose(conn); virConnectClose(dconn); + + if (path != NULL) + unlink(path); + free(path); return s; }

+static CMPIStatus call_external_checks(virDomainPtr dom, + const char *param_path) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + char **list = NULL; + int count = 0; + int i; + + list = list_migration_checks(&count); + if (count < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to execute migration checks"); + goto out; + } else if (list == NULL) { + goto out; + }
Shouldn't you include an error in the list == NULL case as well? -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com

KR> Shouldn't you include an error in the list == NULL case as well? No, I'd like the provider to continue to run even if there are no checks in that directory :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com

Dan Smith wrote:
KR> Shouldn't you include an error in the list == NULL case as well?
No, I'd like the provider to continue to run even if there are no checks in that directory :)
Oh, yep - good call. My mistake. =) -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com

Dan Smith wrote:
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205184074 25200 # Node ID a9134d48bc71c419c78b7e58a71f873281fe84ac # Parent 8eb16f25ec12565c54e553972299fdbdda12533f [RFC] Add external check functionality to CheckIsVSMigratable()
Changes: - Fetch the array out of the MSD ourselves to avoid adding a special case to get_msd_values()
Signed-off-by: Dan Smith <danms@us.ibm.com>
diff -r 8eb16f25ec12 -r a9134d48bc71 src/Virt_VSMigrationService.c --- a/src/Virt_VSMigrationService.c Mon Mar 10 14:20:41 2008 -0700 +++ b/src/Virt_VSMigrationService.c Mon Mar 10 14:21:14 2008 -0700 @@ -21,6 +21,13 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <sys/stat.h> +#include <signal.h> +#include <unistd.h> +#include <dirent.h> +#include <errno.h>
#include <uuid/uuid.h>
@@ -39,6 +46,8 @@ #include "Virt_VSMigrationService.h" #include "Virt_HostSystem.h" #include "Virt_VSMigrationSettingData.h" + +#include "config.h"
#define CIM_JOBSTATE_STARTING 3 #define CIM_JOBSTATE_RUNNING 4 @@ -210,7 +219,6 @@ static CMPIStatus get_msd_values(const C "Failed to connect to remote host (%s)", uri); goto out; } - out: free(uri);
Now what did that blank line do to you to deserve that? Who knows how many compile cycles it's mutely stood witness as all the other lines get to become things like control structures and keywords. And what is its reward for a near eternity of silent service? A summary deletion, with a mere one-character obituary buried in a diff on a mailing list, read by few and noticed by fewer. Well, I noticed, and I won't stand for it!
+static char **list_migration_checks(int *count) +{ + DIR *dir; + struct dirent *de; + char **list = NULL; + int len = 0; + + *count = 0; + + dir = opendir(MIG_CHECKS_DIR); + if (dir == NULL) { + CU_DEBUG("Unable to open migration checks dir: %s (%s)", + MIG_CHECKS_DIR, + strerror(errno)); + *count = -1; + return NULL; + } + + while ((de = readdir(dir)) != NULL) { + int ret; + char *path = NULL; + + if (de->d_name[0] == '.') + continue;
On top of killing the "." and ".." listings, this will also skip all hidden (.foo) files. This is of course fine by me, but just checking that this is the intended behavior.
+static CMPIStatus _call_check(virDomainPtr dom, + const char *prog, + const char *param_path) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + pid_t pid; + int i; + int rc = -1; + + pid = fork(); + if (pid == 0) { + virConnectPtr conn = virDomainGetConnect(dom); + const char *name = virDomainGetName(dom); + const char *uri = virConnectGetURI(conn); + + if (setpgrp() == -1) + perror("setpgrp"); + + execl(prog, prog, name, uri, param_path, NULL); + CU_DEBUG("exec(%s) failed: %s", prog, strerror(errno)); + _exit(1); + } + + for (i = 0; i < (MIG_CHECKS_TIMEOUT * 4); i++) { + int status; + if (waitpid(pid, &status, WNOHANG) != pid) { + usleep(250000); + } else { + rc = WEXITSTATUS(status); + break; + } + } + + if (rc == -1) { + CU_DEBUG("Killing off stale child %i", pid);
One would think the passing of a child would garner a little more emotion, but given the treatment that blank line got, I suppose we should be thankful for what we can get. The rest seems to be typically free of errors, and equally free of heart. -- -Jay

JG> Now what did that blank line do to you to deserve that? Who knows JG> how many compile cycles it's mutely stood witness as all the other JG> lines get to become things like control structures and JG> keywords. And what is its reward for a near eternity of silent JG> service? A summary deletion, with a mere one-character obituary JG> buried in a diff on a mailing list, read by few and noticed by JG> fewer. Well, I noticed, and I won't stand for it! /me stares at the screen in amazement JG> On top of killing the "." and ".." listings, this will also skip JG> all hidden (.foo) files. This is of course fine by me, but just JG> checking that this is the intended behavior. Yes, I think that's a good thing. If someone goes to look in, or clear out that directory to be "safe", hidden files could cause confusion and risk. It's also a reasonable way for people to disable a check temporarily, by making it hidden. A new set without the senseless killing of a blank line is on the way. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com

Dan Smith wrote:
Changes to reflect change to base VSMigrationService MSD change
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
Sorry, I'm not able to apply this patch set to the latest tree. Did I miss a patch set ? -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
participants (4)
-
Dan Smith
-
Heidi Eckhart
-
Jay Gagnon
-
Kaitlin Rupert