[PATCH 0 of 2] Add MSD support to CheckVSIsMigratable()
This adds MSD support to the check functions, as well as adding a check to make sure the domain in question actually exists on the source system.
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205171774 25200 # Node ID 05cbd18bf3644b6e1b32b37c30310fb1b1e23a44 # Parent 1d2157cd04e4d46b50d9e6115e0927d8b0a33b8c Make the migration check functions aware of MSD Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 1d2157cd04e4 -r 05cbd18bf364 src/Virt_VSMigrationService.c --- a/src/Virt_VSMigrationService.c Mon Mar 10 14:40:47 2008 +0100 +++ b/src/Virt_VSMigrationService.c Mon Mar 10 10:56:14 2008 -0700 @@ -68,6 +68,63 @@ struct migration_job { char uuid[33]; }; +static CMPIStatus get_msd(const CMPIObjectPath *ref, + const CMPIArgs *argsin, + CMPIInstance **msd) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + int ret; + + ret = cu_get_inst_arg(argsin, "MigrationSettingData", msd); + if ((ret == CMPI_RC_OK) && (*msd != NULL)) + goto out; + + s = get_migration_sd(ref, msd, _BROKER, false); + if ((s.rc != CMPI_RC_OK) || (*msd == NULL)) { + cu_statusf(_BROKER, &s, + s.rc, + "Unable to get default setting data values"); + goto out; + } + CU_DEBUG("Using default values for MigrationSettingData param"); + + out: + return s; +} + +static CMPIStatus get_migration_type(CMPIInstance *msd, + uint16_t *type) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + int ret; + + ret = cu_get_u16_prop(msd, "MigrationType", type); + if (ret != CMPI_RC_OK) { + cu_statusf(_BROKER, &s, + ret, + "Invalid MigrationType value"); + } + + return s; +} + +static CMPIStatus get_migration_uri(CMPIInstance *msd, + uint16_t *uri) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + int ret; + + ret = cu_get_u16_prop(msd, "TransportType", uri); + if (ret == CMPI_RC_OK) + goto out; + + CU_DEBUG("Using default TransportType: %d", CIM_MIGRATE_URI_SSH); + *uri = CIM_MIGRATE_URI_SSH; + + out: + return s; +} + static char *dest_uri(const char *cn, const char *dest, uint16_t transport) @@ -159,39 +216,67 @@ static CMPIStatus check_hver(virConnectP return s; } +static CMPIStatus get_msd_check_values(const CMPIObjectPath *ref, + const char *destination, + const CMPIArgs *argsin, + virConnectPtr *conn) +{ + CMPIStatus s; + CMPIInstance *msd; + uint16_t uri_type; + char *uri = NULL; + + s = get_msd(ref, argsin, &msd); + if (s.rc != CMPI_RC_OK) + goto out; + + s = get_migration_uri(msd, &uri_type); + if (s.rc != CMPI_RC_OK) + goto out; + + uri = dest_uri(CLASSNAME(ref), destination, uri_type); + if (uri == NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to construct a valid libvirt URI"); + goto out; + } + + *conn = virConnectOpen(uri); + if (*conn == NULL) { + CU_DEBUG("Failed to connect to remote host (%s)", uri); + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to connect to remote host (%s)", uri); + goto out; + } + + out: + free(uri); + + return s; +} + static CMPIStatus vs_migratable(const CMPIObjectPath *ref, const char *domain, const char *destination, const CMPIResult *results, + const CMPIArgs *argsin, CMPIArgs *argsout) { CMPIStatus s; - char *uri = NULL; virConnectPtr conn = NULL; virConnectPtr dconn = NULL; uint32_t retcode = 1; CMPIBoolean isMigratable = 0; - uri = dest_uri(CLASSNAME(ref), destination, CIM_MIGRATE_URI_SSH); - if (uri == NULL) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Failed to construct a valid libvirt URI"); - goto out; - } + s = get_msd_check_values(ref, destination, argsin, &dconn); + if (s.rc != CMPI_RC_OK) + goto out; conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s); if (conn == NULL) goto out; - - dconn = virConnectOpen(uri); - if (dconn == NULL) { - CU_DEBUG("Failed to connect to remote host (%s)", uri); - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Failed to connect to remote host (%s)", uri); - goto out; - } s = check_hver(conn, dconn); if (s.rc != CMPI_RC_OK) @@ -212,7 +297,6 @@ static CMPIStatus vs_migratable(const CM CMAddArg(argsout, "IsMigratable", (CMPIValue *)&isMigratable, CMPI_boolean); - free(uri); virConnectClose(conn); virConnectClose(dconn); @@ -250,7 +334,7 @@ static CMPIStatus vs_migratable_host(CMP return s; } - return vs_migratable(ref, name, dhost, results, argsout); + return vs_migratable(ref, name, dhost, results, argsin, argsout); } static CMPIStatus vs_migratable_system(CMPIMethodMI *self, @@ -293,7 +377,7 @@ static CMPIStatus vs_migratable_system(C return s; } - return vs_migratable(ref, name, dname, results, argsout); + return vs_migratable(ref, name, dname, results, argsin, argsout); } static const char *ind_type_to_name(int ind_type) @@ -771,66 +855,9 @@ static CMPIStatus migrate_create_job_ins return s; } -static CMPIStatus get_msd(const CMPIObjectPath *ref, - const CMPIArgs *argsin, - CMPIInstance **msd) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret; - - ret = cu_get_inst_arg(argsin, "MigrationSettingData", msd); - if ((ret == CMPI_RC_OK) && (*msd != NULL)) - goto out; - - s = get_migration_sd(ref, msd, _BROKER, false); - if ((s.rc != CMPI_RC_OK) || (*msd == NULL)) { - cu_statusf(_BROKER, &s, - s.rc, - "Unable to get default setting data values"); - goto out; - } - CU_DEBUG("Using default values for MigrationSettingData param"); - - out: - return s; -} - -static CMPIStatus get_migration_type(CMPIInstance *msd, - uint16_t *type) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret; - - ret = cu_get_u16_prop(msd, "MigrationType", type); - if (ret != CMPI_RC_OK) { - cu_statusf(_BROKER, &s, - ret, - "Invalid MigrationType value"); - } - - return s; -} - -static CMPIStatus get_migration_uri(CMPIInstance *msd, - uint16_t *uri) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - int ret; - - ret = cu_get_u16_prop(msd, "TransportType", uri); - if (ret == CMPI_RC_OK) - goto out; - - CU_DEBUG("Using default TransportType: %d", CIM_MIGRATE_URI_SSH); - *uri = CIM_MIGRATE_URI_SSH; - - out: - return s; -} - -static CMPIStatus get_msd_values(const CMPIObjectPath *ref, - const CMPIArgs *argsin, - struct migration_job *job) +static CMPIStatus get_msd_job_values(const CMPIObjectPath *ref, + const CMPIArgs *argsin, + struct migration_job *job) { CMPIStatus s; CMPIInstance *msd; @@ -901,7 +928,7 @@ static CMPIStatus migrate_do(const CMPIO goto out; } - s = get_msd_values(ref, argsin, job); + s = get_msd_job_values(ref, argsin, job); if (s.rc != CMPI_RC_OK) goto out;
+static CMPIStatus get_msd_check_values(const CMPIObjectPath *ref,
- -static CMPIStatus get_msd_values(const CMPIObjectPath *ref, - const CMPIArgs *argsin, - struct migration_job *job) +static CMPIStatus get_msd_job_values(const CMPIObjectPath *ref, + const CMPIArgs *argsin, + struct migration_job *job) {
I was thinking maybe you could condense these functions some. Instead of having a transport field in the job struct, you could have a uri field. So, you could have a function that is called get_msd_values() that has everything from get_msd_check_values() except the virConnectPtr / connect_by_classname() part. get_msd_check_values() would just call get_msd_values() plus the connect_by_classname() bit. Then the check functions could call get_msd_check_values() and the migrate functions could call get_msd_values(). Probably not worth the reordering and work though. Otherwise, I have no issues here. This set tested fine on my system. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
KR> So, you could have a function that is called get_msd_values() that has KR> everything from get_msd_check_values() except the virConnectPtr / KR> connect_by_classname() part. get_msd_check_values() would just call KR> get_msd_values() plus the connect_by_classname() bit. There is also the array processing that the check version does. I experimented with this a little and we ended up having a little overlap and then a special case for both. Given the rather small size and the possibility for more special cases later, it didn't seem worth the mess of combining them. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205171774 25200 # Node ID 29e665fff096426ced0f3c7c9ddc7c0924927a07 # Parent 05cbd18bf3644b6e1b32b37c30310fb1b1e23a44 Add lookup of domain to make sure it exists as part of the check Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 05cbd18bf364 -r 29e665fff096 src/Virt_VSMigrationService.c --- a/src/Virt_VSMigrationService.c Mon Mar 10 10:56:14 2008 -0700 +++ b/src/Virt_VSMigrationService.c Mon Mar 10 10:56:14 2008 -0700 @@ -269,6 +269,7 @@ static CMPIStatus vs_migratable(const CM virConnectPtr dconn = NULL; uint32_t retcode = 1; CMPIBoolean isMigratable = 0; + virDomainPtr dom = NULL; s = get_msd_check_values(ref, destination, argsin, &dconn); if (s.rc != CMPI_RC_OK) @@ -281,6 +282,14 @@ static CMPIStatus vs_migratable(const CM s = check_hver(conn, dconn); if (s.rc != CMPI_RC_OK) goto out; + + dom = virDomainLookupByName(conn, domain); + if (dom == NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such domain"); + goto out; + } s = check_caps(conn, dconn); if (s.rc != CMPI_RC_OK) @@ -297,6 +306,7 @@ static CMPIStatus vs_migratable(const CM CMAddArg(argsout, "IsMigratable", (CMPIValue *)&isMigratable, CMPI_boolean); + virDomainFree(dom); virConnectClose(conn); virConnectClose(dconn);
Dan Smith wrote:
@@ -281,6 +282,14 @@ static CMPIStatus vs_migratable(const CM s = check_hver(conn, dconn); if (s.rc != CMPI_RC_OK) goto out; + + dom = virDomainLookupByName(conn, domain); + if (dom == NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such domain"); + goto out; + }
Sorry, I know my comment comes a bit late, as the patch is already checked in. I think this check is not enough. To check if a certain Xen/KVM_ComputerSystem or the Xen/KVM_HostSystem object path is correct specified by the client, you should use the corresponding (new ;)) interfaces (get_domain_by_ref, get_host), as they check the complete object path for correctness. It raises the number of cycles a bit, but makes the behavior of all providers consistent. Thanks. -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
HE> Sorry, I know my comment comes a bit late, as the patch is already HE> checked in. That's okay :) HE> I think this check is not enough. To check if a certain HE> Xen/KVM_ComputerSystem or the Xen/KVM_HostSystem object path is HE> correct specified by the client, you should use the corresponding HE> (new ;)) interfaces (get_domain_by_ref, get_host), as they check HE> the complete object path for correctness. It raises the number of HE> cycles a bit, but makes the behavior of all providers HE> consistent. Well, the reason I added this is twofold: (1) because of the fact that we would fail late if the domain doesn't exist, and (2) because I use the dom pointer later for the external check code. So, I'm more than happy to add in the more rigorous checks, but we need this part as well :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> Sorry, I know my comment comes a bit late, as the patch is already HE> checked in.
That's okay :)
HE> I think this check is not enough. To check if a certain HE> Xen/KVM_ComputerSystem or the Xen/KVM_HostSystem object path is HE> correct specified by the client, you should use the corresponding HE> (new ;)) interfaces (get_domain_by_ref, get_host), as they check HE> the complete object path for correctness. It raises the number of HE> cycles a bit, but makes the behavior of all providers HE> consistent.
Well, the reason I added this is twofold: (1) because of the fact that we would fail late if the domain doesn't exist, and (2) because I use the dom pointer later for the external check code.
So, I'm more than happy to add in the more rigorous checks, but we need this part as well :)
ok, fine for me :) -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
participants (3)
-
Dan Smith -
Heidi Eckhart -
Kaitlin Rupert