[PATCH V5 00/15] Bug fix patches for 0.6.2

This serial fix a serial of issues. V5: General change: Remove script change patch, need John Ferlan's patch in following. 1/15-3/15: new patch make libvirt-cim conform to DSP more. 6/15: better commit message. 8/15: remove a strdup() to reduce a error path. 10/15: free *xml after a fail. 11/15: return fail when ind == NULL in trigger_mod_indiction(), check return value for calloc() in doms_to_xml(), better debug message, check strdup() return on raise_indication, better traverse in dom list. Reduce condition macros. 12/15: new patch to move CSI-libvirt/CSI-libvirt-cim together. 13/15: remove defensive code, only fix the problem of report fail when some VSSD succeed. 14/15: new separate patch from 15/15, to enhance config reading. 15/15: changed the interface, new method to copy/delete the key, rm the key before copy and check the key's status after copy. Wenchao Xia (15): 1 Remove property CreationClassName in some instance 2 VSSD: add missing property IsFullVirt in schema 3 SDC: use property BootDevices instead of BootDevice 4 do not deregister virt classes in yum upgrade 5 CSI, fix debug print crash 6 CSI, add lock to protect shared data in lifecycle_thread 7 DevicePool, fix debug print crash 8 DevicePool, reimplement get_diskpool_config with libvirt 9 RASDIndication, fix debug print crash 10 device parsing, add debug print 11 CSI Discard libvirt event by default 12 CSI: Move native CSI code together 13 VSSD: report success if not all VS fail in enum 14 misc_util: better way to read config 15 migration: allow ssh based migration with non root's key file libvirt-cim.conf | 19 + libvirt-cim.spec.in | 12 +- libxkutil/device_parsing.c | 16 +- libxkutil/misc_util.c | 135 +++- libxkutil/misc_util.h | 6 +- schema/Virt_VSSD.mof | 3 + src/Virt_AllocationCapabilities.c | 3 +- src/Virt_ComputerSystem.c | 66 ++- src/Virt_ComputerSystemIndication.c | 1018 +++++++++++++++++--- src/Virt_ConsoleRedirectionService.c | 3 +- src/Virt_ConsoleRedirectionServiceCapabilities.c | 3 +- src/Virt_Device.c | 18 +- src/Virt_DevicePool.c | 198 +++-- src/Virt_EnabledLogicalElementCapabilities.c | 3 +- src/Virt_FilterEntry.c | 3 +- src/Virt_FilterList.c | 3 +- src/Virt_HostSystem.c | 3 +- src/Virt_KVMRedirectionSAP.c | 3 +- src/Virt_RASD.c | 3 +- src/Virt_ReferencedProfile.c | 3 +- src/Virt_RegisteredProfile.c | 3 +- src/Virt_ResourceAllocationSettingDataIndication.c | 6 +- src/Virt_ResourcePoolConfigurationCapabilities.c | 3 +- src/Virt_ResourcePoolConfigurationService.c | 8 +- src/Virt_SettingsDefineCapabilities.c | 10 +- src/Virt_SwitchService.c | 3 +- src/Virt_VSMigrationCapabilities.c | 3 +- src/Virt_VSMigrationService.c | 269 +++++- src/Virt_VSMigrationSettingData.c | 3 +- src/Virt_VSSD.c | 46 +- src/Virt_VirtualSystemManagementCapabilities.c | 3 +- src/Virt_VirtualSystemManagementService.c | 63 ++- src/Virt_VirtualSystemSnapshotService.c | 3 +- ...Virt_VirtualSystemSnapshotServiceCapabilities.c | 3 +- 34 files changed, 1652 insertions(+), 295 deletions(-)

There are some instances which did not register this property, so this patch added a parameter in get_typed_instance(). The caller must set it clearly whether to set the property. When tog-pegasus > 2.12 and its log >= WARNING, tog-pegasus will report this problem in log. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libxkutil/misc_util.c | 12 ++++++++---- libxkutil/misc_util.h | 3 ++- src/Virt_AllocationCapabilities.c | 3 ++- src/Virt_ComputerSystem.c | 6 ++++-- src/Virt_ComputerSystemIndication.c | 3 ++- src/Virt_ConsoleRedirectionService.c | 3 ++- src/Virt_ConsoleRedirectionServiceCapabilities.c | 3 ++- src/Virt_Device.c | 18 ++++++++++++------ src/Virt_DevicePool.c | 20 +++++++++++++------- src/Virt_EnabledLogicalElementCapabilities.c | 3 ++- src/Virt_FilterEntry.c | 3 ++- src/Virt_FilterList.c | 3 ++- src/Virt_HostSystem.c | 3 ++- src/Virt_KVMRedirectionSAP.c | 3 ++- src/Virt_RASD.c | 3 ++- src/Virt_ReferencedProfile.c | 3 ++- src/Virt_RegisteredProfile.c | 3 ++- src/Virt_ResourcePoolConfigurationCapabilities.c | 3 ++- src/Virt_ResourcePoolConfigurationService.c | 8 ++++++-- src/Virt_SettingsDefineCapabilities.c | 6 ++++-- src/Virt_SwitchService.c | 3 ++- src/Virt_VSMigrationCapabilities.c | 3 ++- src/Virt_VSMigrationService.c | 6 ++++-- src/Virt_VSMigrationSettingData.c | 3 ++- src/Virt_VSSD.c | 3 ++- src/Virt_VirtualSystemManagementCapabilities.c | 3 ++- src/Virt_VirtualSystemManagementService.c | 6 ++++-- src/Virt_VirtualSystemSnapshotService.c | 3 ++- ...Virt_VirtualSystemSnapshotServiceCapabilities.c | 3 ++- 29 files changed, 97 insertions(+), 48 deletions(-) diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index 2d149ae..f1b93e4 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -341,7 +341,8 @@ char *get_typed_class(const char *refcn, const char *new_base) CMPIInstance *get_typed_instance(const CMPIBroker *broker, const char *refcn, const char *base, - const char *namespace) + const char *namespace, + bool ccn_flag) { char *new_cn; CMPIObjectPath *op; @@ -360,8 +361,10 @@ CMPIInstance *get_typed_instance(const CMPIBroker *broker, if ((s.rc != CMPI_RC_OK) || CMIsNullObject(inst)) goto out; - CMSetProperty(inst, "CreationClassName", - (CMPIValue *)new_cn, CMPI_chars); + if (ccn_flag) { + CMSetProperty(inst, "CreationClassName", + (CMPIValue *)new_cn, CMPI_chars); + } out: free(new_cn); @@ -467,7 +470,8 @@ CMPIInstance *make_reference(const CMPIBroker *broker, ref_inst = get_typed_instance(broker, CLASSNAME(source_ref), assoc_classname, - NAMESPACE(source_ref)); + NAMESPACE(source_ref), + false); if (ref_inst != NULL) { CMPIObjectPath *target_ref; diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h index c7a2122..90fb2da 100644 --- a/libxkutil/misc_util.h +++ b/libxkutil/misc_util.h @@ -85,7 +85,8 @@ char *get_typed_class(const char *refcn, const char *new_base); CMPIInstance *get_typed_instance(const CMPIBroker *broker, const char *refcn, const char *base, - const char *namespace); + const char *namespace, + bool ccn_flag); /* Parse an OrgID:LocID string into its constituent parts */ int parse_instance_id(char *iid, char **orgid, char **locid); diff --git a/src/Virt_AllocationCapabilities.c b/src/Virt_AllocationCapabilities.c index 970abeb..b358fac 100644 --- a/src/Virt_AllocationCapabilities.c +++ b/src/Virt_AllocationCapabilities.c @@ -46,7 +46,8 @@ static CMPIStatus ac_from_pool(const CMPIBroker *broker, *alloc_cap = get_typed_instance(broker, CLASSNAME(ref), "AllocationCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (*alloc_cap == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index e6c7e55..4a9b26d 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -440,7 +440,8 @@ CMPIStatus instance_from_dominfo(const CMPIBroker *broker, inst = get_typed_instance(broker, prefix, "ComputerSystem", - namespace); + namespace, + true); if (inst == NULL) { CU_DEBUG("Could not init CS instance. " @@ -560,7 +561,8 @@ static CMPIStatus instance_from_dom(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "ComputerSystem", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 2d0a94e..3096683 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -239,7 +239,8 @@ static bool _do_indication(const CMPIBroker *broker, ind = get_typed_instance(broker, prefix, ind_type_name, - args->ns); + args->ns, + false); /* Generally report errors and hope to continue, since we have no one to actually return status to. */ diff --git a/src/Virt_ConsoleRedirectionService.c b/src/Virt_ConsoleRedirectionService.c index 7b20c0d..16cade8 100644 --- a/src/Virt_ConsoleRedirectionService.c +++ b/src/Virt_ConsoleRedirectionService.c @@ -129,7 +129,8 @@ CMPIStatus get_console_rs(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "ConsoleRedirectionService", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_ConsoleRedirectionServiceCapabilities.c b/src/Virt_ConsoleRedirectionServiceCapabilities.c index 11f2986..88fb3a4 100644 --- a/src/Virt_ConsoleRedirectionServiceCapabilities.c +++ b/src/Virt_ConsoleRedirectionServiceCapabilities.c @@ -91,7 +91,8 @@ CMPIStatus get_console_rs_caps(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "ConsoleRedirectionServiceCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_Device.c b/src/Virt_Device.c index e047a94..c7dcbc3 100644 --- a/src/Virt_Device.c +++ b/src/Virt_Device.c @@ -100,7 +100,8 @@ static CMPIInstance *net_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "NetworkPort", - ns); + ns, + true); if (inst == NULL) { CU_DEBUG("Failed to get instance for NetworkPort"); @@ -138,7 +139,8 @@ static CMPIInstance *disk_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "LogicalDisk", - ns); + ns, + true); if (inst == NULL) { CU_DEBUG("Failed to get instance for LogicalDisk"); @@ -185,7 +187,8 @@ static CMPIInstance *mem_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "Memory", - ns); + ns, + true); if (inst == NULL) { CU_DEBUG("Failed to get instance for Memory"); @@ -234,7 +237,8 @@ static CMPIInstance *graphics_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "DisplayController", - ns); + ns, + true); if (inst == NULL) { CU_DEBUG("Failed to get instance for DisplayController"); @@ -320,7 +324,8 @@ static CMPIInstance *input_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "PointingDevice", - ns); + ns, + true); if (inst == NULL) { CU_DEBUG("Failed to get instance of %s_PointingDevice", pfx_from_conn(conn)); @@ -407,7 +412,8 @@ static bool vcpu_inst(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "Processor", - ns); + ns, + true); if (inst == NULL) return false; diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 202e509..56c9715 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -803,7 +803,8 @@ static CMPIStatus mempool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "MemoryPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, @@ -842,7 +843,8 @@ static CMPIStatus procpool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "ProcessorPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, @@ -872,7 +874,8 @@ static CMPIStatus _netpool_for_parent(struct inst_list *list, inst = get_typed_instance(broker, refcn, "NetworkPool", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Unable to get instance: %s:%s_NetworkPool", ns, refcn); @@ -928,7 +931,8 @@ static CMPIStatus _netpool_for_network(struct inst_list *list, inst = get_typed_instance(broker, refcn, "NetworkPool", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Unable to get instance: %s:%s_NetworkPool", ns, refcn); @@ -1046,7 +1050,7 @@ static CMPIInstance *diskpool_from_path(struct tmp_disk_pool *pool, CMPIInstance *inst; char *poolid = NULL; - inst = get_typed_instance(broker, refcn, "DiskPool", ns); + inst = get_typed_instance(broker, refcn, "DiskPool", ns, false); if (inst == NULL) { cu_statusf(broker, &s, @@ -1136,7 +1140,8 @@ static CMPIStatus graphicspool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "GraphicsPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, @@ -1172,7 +1177,8 @@ static CMPIStatus inputpool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "InputPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_EnabledLogicalElementCapabilities.c b/src/Virt_EnabledLogicalElementCapabilities.c index 7ba5eae..2fcdef1 100644 --- a/src/Virt_EnabledLogicalElementCapabilities.c +++ b/src/Virt_EnabledLogicalElementCapabilities.c @@ -61,7 +61,8 @@ static CMPIInstance *_get_elec(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "EnabledLogicalElementCapabilities", - NAMESPACE(reference)); + NAMESPACE(reference), + false); if (inst == NULL) { cu_statusf(broker, s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index 126615b..3c4a3e6 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -546,7 +546,8 @@ static CMPIInstance *convert_rule_to_instance( inst = get_typed_instance(broker, CLASSNAME(reference), basename, - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_FilterList.c b/src/Virt_FilterList.c index 9b5dbae..79776cd 100644 --- a/src/Virt_FilterList.c +++ b/src/Virt_FilterList.c @@ -50,7 +50,8 @@ static CMPIInstance *convert_filter_to_instance( inst = get_typed_instance(broker, CLASSNAME(reference), "FilterList", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_HostSystem.c b/src/Virt_HostSystem.c index 724a5ea..c31d6cf 100644 --- a/src/Virt_HostSystem.c +++ b/src/Virt_HostSystem.c @@ -135,7 +135,8 @@ static CMPIStatus fake_host(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "HostSystem", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, &s, diff --git a/src/Virt_KVMRedirectionSAP.c b/src/Virt_KVMRedirectionSAP.c index db34d57..38ad468 100644 --- a/src/Virt_KVMRedirectionSAP.c +++ b/src/Virt_KVMRedirectionSAP.c @@ -125,7 +125,8 @@ static CMPIInstance *get_console_sap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "KVMRedirectionSAP", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index 9493077..6e8a244 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -716,7 +716,8 @@ CMPIInstance *rasd_from_vdev(const CMPIBroker *broker, inst = get_typed_instance(broker, CLASSNAME(ref), base, - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) return inst; diff --git a/src/Virt_ReferencedProfile.c b/src/Virt_ReferencedProfile.c index 150be1f..e78a8d3 100644 --- a/src/Virt_ReferencedProfile.c +++ b/src/Virt_ReferencedProfile.c @@ -242,7 +242,8 @@ static CMPIInstance *make_ref(const CMPIObjectPath *source_ref, ref_inst = get_typed_instance(_BROKER, CLASSNAME(source_ref), assoc_classname, - NAMESPACE(source_ref)); + NAMESPACE(source_ref), + false); source = get_reg_prof_by_ref(source_ref); if (source->scoping_profile != NULL) diff --git a/src/Virt_RegisteredProfile.c b/src/Virt_RegisteredProfile.c index 389a179..e644708 100644 --- a/src/Virt_RegisteredProfile.c +++ b/src/Virt_RegisteredProfile.c @@ -54,7 +54,8 @@ CMPIStatus get_profile(const CMPIBroker *broker, instance = get_typed_instance(broker, pfx, "RegisteredProfile", - CIM_INTEROP_NS); + CIM_INTEROP_NS, + false); if (instance == NULL) { cu_statusf(broker, &s, diff --git a/src/Virt_ResourcePoolConfigurationCapabilities.c b/src/Virt_ResourcePoolConfigurationCapabilities.c index 32274ed..2dcbbcf 100644 --- a/src/Virt_ResourcePoolConfigurationCapabilities.c +++ b/src/Virt_ResourcePoolConfigurationCapabilities.c @@ -69,7 +69,8 @@ static CMPIStatus get_rpc_cap(const CMPIObjectPath *reference, inst = get_typed_instance(_BROKER, pfx_from_conn(conn), "ResourcePoolConfigurationCapabilities", - NAMESPACE(reference)); + NAMESPACE(reference), + false); if (inst == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ResourcePoolConfigurationService.c b/src/Virt_ResourcePoolConfigurationService.c index 751d016..0c0cc06 100644 --- a/src/Virt_ResourcePoolConfigurationService.c +++ b/src/Virt_ResourcePoolConfigurationService.c @@ -781,6 +781,8 @@ static const char *rasd_to_res(CMPIInstance *inst, return msg; } +/* Warning: returned instance is not freed manually in caller, need confirm + if server will auto free it. */ static CMPIInstance *get_resource_rasd(struct virt_pool_res *res, const CMPIObjectPath *ref, CMPIStatus *s) @@ -798,7 +800,8 @@ static CMPIInstance *get_resource_rasd(struct virt_pool_res *res, inst = get_typed_instance(_BROKER, CLASSNAME(ref), "StorageVolumeResourceAllocationSettingData", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, @@ -1279,7 +1282,8 @@ CMPIStatus get_rpcs(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "ResourcePoolConfigurationService", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index 9eb9e57..5091205 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -113,7 +113,8 @@ static CMPIInstance *default_vssd_instance(const char *prefix, inst = get_typed_instance(_BROKER, prefix, "VirtualSystemSettingData", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Failed to create default VSSD instance"); goto out; @@ -303,7 +304,8 @@ static CMPIInstance *sdc_rasd_inst(CMPIStatus *s, inst = get_typed_instance(_BROKER, CLASSNAME(ref), base, - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(_BROKER, s, diff --git a/src/Virt_SwitchService.c b/src/Virt_SwitchService.c index 7e59d38..8991426 100644 --- a/src/Virt_SwitchService.c +++ b/src/Virt_SwitchService.c @@ -229,7 +229,8 @@ static CMPIStatus get_switchservice(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "SwitchService", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_VSMigrationCapabilities.c b/src/Virt_VSMigrationCapabilities.c index 4f0e434..3e53f68 100644 --- a/src/Virt_VSMigrationCapabilities.c +++ b/src/Virt_VSMigrationCapabilities.c @@ -134,7 +134,8 @@ CMPIStatus get_migration_caps(const CMPIObjectPath *ref, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemMigrationCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 76e3d25..1f6659d 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -831,7 +831,8 @@ static CMPIInstance *prepare_indication(const CMPIBroker *broker, ind = get_typed_instance(broker, pfx, ind_name, - job->ref_ns); + job->ref_ns, + false); if (ind == NULL) { CU_DEBUG("Failed to create ind, type '%s:%s_%s'", job->ref_ns, pfx, ind_name); @@ -1686,7 +1687,8 @@ CMPIStatus get_migration_service(const CMPIObjectPath *ref, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemMigrationService", - NAMESPACE(ref)); + NAMESPACE(ref), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSMigrationSettingData.c b/src/Virt_VSMigrationSettingData.c index a6707bd..eb545f1 100644 --- a/src/Virt_VSMigrationSettingData.c +++ b/src/Virt_VSMigrationSettingData.c @@ -79,7 +79,8 @@ CMPIStatus get_migration_sd(const CMPIObjectPath *ref, inst = get_typed_instance(broker, CLASSNAME(ref), "VirtualSystemMigrationSettingData", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 499b157..975623b 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -285,7 +285,8 @@ static CMPIInstance *_get_vssd(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSettingData", - NAMESPACE(reference)); + NAMESPACE(reference), + false); if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_VirtualSystemManagementCapabilities.c b/src/Virt_VirtualSystemManagementCapabilities.c index 020ce8a..51738ee 100644 --- a/src/Virt_VirtualSystemManagementCapabilities.c +++ b/src/Virt_VirtualSystemManagementCapabilities.c @@ -129,7 +129,8 @@ CMPIStatus get_vsm_cap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemManagementCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 10adf8b..96c8a03 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -1928,7 +1928,8 @@ static CMPIStatus raise_rasd_indication(const CMPIContext *context, ind = get_typed_instance(_BROKER, CLASSNAME(ref), base_type, - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (ind == NULL) { CU_DEBUG("Failed to get indication instance"); s.rc = CMPI_RC_ERR_FAILED; @@ -3293,7 +3294,8 @@ CMPIStatus get_vsms(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemManagementService", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_VirtualSystemSnapshotService.c b/src/Virt_VirtualSystemSnapshotService.c index aae628f..8c0889d 100644 --- a/src/Virt_VirtualSystemSnapshotService.c +++ b/src/Virt_VirtualSystemSnapshotService.c @@ -681,7 +681,8 @@ CMPIStatus get_vsss(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSnapshotService", - NAMESPACE(ref)); + NAMESPACE(ref), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VirtualSystemSnapshotServiceCapabilities.c b/src/Virt_VirtualSystemSnapshotServiceCapabilities.c index 69c8e97..04a9c7a 100644 --- a/src/Virt_VirtualSystemSnapshotServiceCapabilities.c +++ b/src/Virt_VirtualSystemSnapshotServiceCapabilities.c @@ -117,7 +117,8 @@ CMPIStatus get_vss_cap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSnapshotServiceCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
There are some instances which did not register this property, so this patch added a parameter in get_typed_instance(). The caller must set it clearly whether to set the property. When tog-pegasus > 2.12 and its log >= WARNING, tog-pegasus will report this problem in log.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libxkutil/misc_util.c | 12 ++++++++---- libxkutil/misc_util.h | 3 ++- src/Virt_AllocationCapabilities.c | 3 ++- src/Virt_ComputerSystem.c | 6 ++++-- src/Virt_ComputerSystemIndication.c | 3 ++- src/Virt_ConsoleRedirectionService.c | 3 ++- src/Virt_ConsoleRedirectionServiceCapabilities.c | 3 ++- src/Virt_Device.c | 18 ++++++++++++------ src/Virt_DevicePool.c | 20 +++++++++++++------- src/Virt_EnabledLogicalElementCapabilities.c | 3 ++- src/Virt_FilterEntry.c | 3 ++- src/Virt_FilterList.c | 3 ++- src/Virt_HostSystem.c | 3 ++- src/Virt_KVMRedirectionSAP.c | 3 ++- src/Virt_RASD.c | 3 ++- src/Virt_ReferencedProfile.c | 3 ++- src/Virt_RegisteredProfile.c | 3 ++- src/Virt_ResourcePoolConfigurationCapabilities.c | 3 ++- src/Virt_ResourcePoolConfigurationService.c | 8 ++++++-- src/Virt_SettingsDefineCapabilities.c | 6 ++++-- src/Virt_SwitchService.c | 3 ++- src/Virt_VSMigrationCapabilities.c | 3 ++- src/Virt_VSMigrationService.c | 6 ++++-- src/Virt_VSMigrationSettingData.c | 3 ++- src/Virt_VSSD.c | 3 ++- src/Virt_VirtualSystemManagementCapabilities.c | 3 ++- src/Virt_VirtualSystemManagementService.c | 6 ++++-- src/Virt_VirtualSystemSnapshotService.c | 3 ++- ...Virt_VirtualSystemSnapshotServiceCapabilities.c | 3 ++- 29 files changed, 97 insertions(+), 48 deletions(-)
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index 2d149ae..f1b93e4 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -341,7 +341,8 @@ char *get_typed_class(const char *refcn, const char *new_base) CMPIInstance *get_typed_instance(const CMPIBroker *broker, const char *refcn, const char *base, - const char *namespace) + const char *namespace, + bool ccn_flag)
While it may seem obvious to you why one would want to provide 'true' or 'false' here, could the function header elaborate a bit so that in the future when someone "adds" a call to this function they will know whether they should pass true or false. IOW which type of instances need this and which do not. If the answer is "as according to the DSP" - I think that's fine. I don't know where to find the DSP (yet) or at least where/how to make the determination that the caller would (or not) need the property.
From my scan of these - it sems *Capabilities, *Indications, *Filter, *Profiles, & *RASDs type classes don't need the property, while specific element classes (ComputerSystem, HostSystem, LogicalDisk, NetworkPort, etc) and *Service classes need the property. Is that a "fair" summary?
ACK with description the remainder looks fine. John
{ char *new_cn; CMPIObjectPath *op; @@ -360,8 +361,10 @@ CMPIInstance *get_typed_instance(const CMPIBroker *broker, if ((s.rc != CMPI_RC_OK) || CMIsNullObject(inst)) goto out;
- CMSetProperty(inst, "CreationClassName", - (CMPIValue *)new_cn, CMPI_chars); + if (ccn_flag) { + CMSetProperty(inst, "CreationClassName", + (CMPIValue *)new_cn, CMPI_chars); + }
out: free(new_cn); @@ -467,7 +470,8 @@ CMPIInstance *make_reference(const CMPIBroker *broker, ref_inst = get_typed_instance(broker, CLASSNAME(source_ref), assoc_classname, - NAMESPACE(source_ref)); + NAMESPACE(source_ref), + false);
if (ref_inst != NULL) { CMPIObjectPath *target_ref; diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h index c7a2122..90fb2da 100644 --- a/libxkutil/misc_util.h +++ b/libxkutil/misc_util.h @@ -85,7 +85,8 @@ char *get_typed_class(const char *refcn, const char *new_base); CMPIInstance *get_typed_instance(const CMPIBroker *broker, const char *refcn, const char *base, - const char *namespace); + const char *namespace, + bool ccn_flag);
/* Parse an OrgID:LocID string into its constituent parts */ int parse_instance_id(char *iid, char **orgid, char **locid); diff --git a/src/Virt_AllocationCapabilities.c b/src/Virt_AllocationCapabilities.c index 970abeb..b358fac 100644 --- a/src/Virt_AllocationCapabilities.c +++ b/src/Virt_AllocationCapabilities.c @@ -46,7 +46,8 @@ static CMPIStatus ac_from_pool(const CMPIBroker *broker, *alloc_cap = get_typed_instance(broker, CLASSNAME(ref), "AllocationCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (*alloc_cap == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index e6c7e55..4a9b26d 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -440,7 +440,8 @@ CMPIStatus instance_from_dominfo(const CMPIBroker *broker, inst = get_typed_instance(broker, prefix, "ComputerSystem", - namespace); + namespace, + true);
if (inst == NULL) { CU_DEBUG("Could not init CS instance. " @@ -560,7 +561,8 @@ static CMPIStatus instance_from_dom(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "ComputerSystem", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 2d0a94e..3096683 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -239,7 +239,8 @@ static bool _do_indication(const CMPIBroker *broker, ind = get_typed_instance(broker, prefix, ind_type_name, - args->ns); + args->ns, + false);
/* Generally report errors and hope to continue, since we have no one to actually return status to. */ diff --git a/src/Virt_ConsoleRedirectionService.c b/src/Virt_ConsoleRedirectionService.c index 7b20c0d..16cade8 100644 --- a/src/Virt_ConsoleRedirectionService.c +++ b/src/Virt_ConsoleRedirectionService.c @@ -129,7 +129,8 @@ CMPIStatus get_console_rs(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "ConsoleRedirectionService", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_ConsoleRedirectionServiceCapabilities.c b/src/Virt_ConsoleRedirectionServiceCapabilities.c index 11f2986..88fb3a4 100644 --- a/src/Virt_ConsoleRedirectionServiceCapabilities.c +++ b/src/Virt_ConsoleRedirectionServiceCapabilities.c @@ -91,7 +91,8 @@ CMPIStatus get_console_rs_caps(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "ConsoleRedirectionServiceCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_Device.c b/src/Virt_Device.c index e047a94..c7dcbc3 100644 --- a/src/Virt_Device.c +++ b/src/Virt_Device.c @@ -100,7 +100,8 @@ static CMPIInstance *net_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "NetworkPort", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for NetworkPort"); @@ -138,7 +139,8 @@ static CMPIInstance *disk_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "LogicalDisk", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for LogicalDisk"); @@ -185,7 +187,8 @@ static CMPIInstance *mem_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "Memory", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for Memory"); @@ -234,7 +237,8 @@ static CMPIInstance *graphics_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "DisplayController", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for DisplayController"); @@ -320,7 +324,8 @@ static CMPIInstance *input_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "PointingDevice", - ns); + ns, + true); if (inst == NULL) { CU_DEBUG("Failed to get instance of %s_PointingDevice", pfx_from_conn(conn)); @@ -407,7 +412,8 @@ static bool vcpu_inst(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "Processor", - ns); + ns, + true); if (inst == NULL) return false;
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 202e509..56c9715 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -803,7 +803,8 @@ static CMPIStatus mempool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "MemoryPool", - ns); + ns, + false);
if (inst == NULL) { cu_statusf(broker, &s, @@ -842,7 +843,8 @@ static CMPIStatus procpool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "ProcessorPool", - ns); + ns, + false);
if (inst == NULL) { cu_statusf(broker, &s, @@ -872,7 +874,8 @@ static CMPIStatus _netpool_for_parent(struct inst_list *list, inst = get_typed_instance(broker, refcn, "NetworkPool", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Unable to get instance: %s:%s_NetworkPool", ns, refcn); @@ -928,7 +931,8 @@ static CMPIStatus _netpool_for_network(struct inst_list *list, inst = get_typed_instance(broker, refcn, "NetworkPool", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Unable to get instance: %s:%s_NetworkPool", ns, refcn); @@ -1046,7 +1050,7 @@ static CMPIInstance *diskpool_from_path(struct tmp_disk_pool *pool, CMPIInstance *inst; char *poolid = NULL;
- inst = get_typed_instance(broker, refcn, "DiskPool", ns); + inst = get_typed_instance(broker, refcn, "DiskPool", ns, false);
if (inst == NULL) { cu_statusf(broker, &s, @@ -1136,7 +1140,8 @@ static CMPIStatus graphicspool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "GraphicsPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, @@ -1172,7 +1177,8 @@ static CMPIStatus inputpool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "InputPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_EnabledLogicalElementCapabilities.c b/src/Virt_EnabledLogicalElementCapabilities.c index 7ba5eae..2fcdef1 100644 --- a/src/Virt_EnabledLogicalElementCapabilities.c +++ b/src/Virt_EnabledLogicalElementCapabilities.c @@ -61,7 +61,8 @@ static CMPIInstance *_get_elec(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "EnabledLogicalElementCapabilities", - NAMESPACE(reference)); + NAMESPACE(reference), + false); if (inst == NULL) { cu_statusf(broker, s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index 126615b..3c4a3e6 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -546,7 +546,8 @@ static CMPIInstance *convert_rule_to_instance( inst = get_typed_instance(broker, CLASSNAME(reference), basename, - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_FilterList.c b/src/Virt_FilterList.c index 9b5dbae..79776cd 100644 --- a/src/Virt_FilterList.c +++ b/src/Virt_FilterList.c @@ -50,7 +50,8 @@ static CMPIInstance *convert_filter_to_instance( inst = get_typed_instance(broker, CLASSNAME(reference), "FilterList", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_HostSystem.c b/src/Virt_HostSystem.c index 724a5ea..c31d6cf 100644 --- a/src/Virt_HostSystem.c +++ b/src/Virt_HostSystem.c @@ -135,7 +135,8 @@ static CMPIStatus fake_host(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "HostSystem", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { cu_statusf(broker, &s, diff --git a/src/Virt_KVMRedirectionSAP.c b/src/Virt_KVMRedirectionSAP.c index db34d57..38ad468 100644 --- a/src/Virt_KVMRedirectionSAP.c +++ b/src/Virt_KVMRedirectionSAP.c @@ -125,7 +125,8 @@ static CMPIInstance *get_console_sap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "KVMRedirectionSAP", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index 9493077..6e8a244 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -716,7 +716,8 @@ CMPIInstance *rasd_from_vdev(const CMPIBroker *broker, inst = get_typed_instance(broker, CLASSNAME(ref), base, - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) return inst;
diff --git a/src/Virt_ReferencedProfile.c b/src/Virt_ReferencedProfile.c index 150be1f..e78a8d3 100644 --- a/src/Virt_ReferencedProfile.c +++ b/src/Virt_ReferencedProfile.c @@ -242,7 +242,8 @@ static CMPIInstance *make_ref(const CMPIObjectPath *source_ref, ref_inst = get_typed_instance(_BROKER, CLASSNAME(source_ref), assoc_classname, - NAMESPACE(source_ref)); + NAMESPACE(source_ref), + false);
source = get_reg_prof_by_ref(source_ref); if (source->scoping_profile != NULL) diff --git a/src/Virt_RegisteredProfile.c b/src/Virt_RegisteredProfile.c index 389a179..e644708 100644 --- a/src/Virt_RegisteredProfile.c +++ b/src/Virt_RegisteredProfile.c @@ -54,7 +54,8 @@ CMPIStatus get_profile(const CMPIBroker *broker, instance = get_typed_instance(broker, pfx, "RegisteredProfile", - CIM_INTEROP_NS); + CIM_INTEROP_NS, + false);
if (instance == NULL) { cu_statusf(broker, &s, diff --git a/src/Virt_ResourcePoolConfigurationCapabilities.c b/src/Virt_ResourcePoolConfigurationCapabilities.c index 32274ed..2dcbbcf 100644 --- a/src/Virt_ResourcePoolConfigurationCapabilities.c +++ b/src/Virt_ResourcePoolConfigurationCapabilities.c @@ -69,7 +69,8 @@ static CMPIStatus get_rpc_cap(const CMPIObjectPath *reference, inst = get_typed_instance(_BROKER, pfx_from_conn(conn), "ResourcePoolConfigurationCapabilities", - NAMESPACE(reference)); + NAMESPACE(reference), + false); if (inst == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ResourcePoolConfigurationService.c b/src/Virt_ResourcePoolConfigurationService.c index 751d016..0c0cc06 100644 --- a/src/Virt_ResourcePoolConfigurationService.c +++ b/src/Virt_ResourcePoolConfigurationService.c @@ -781,6 +781,8 @@ static const char *rasd_to_res(CMPIInstance *inst, return msg; }
+/* Warning: returned instance is not freed manually in caller, need confirm + if server will auto free it. */ static CMPIInstance *get_resource_rasd(struct virt_pool_res *res, const CMPIObjectPath *ref, CMPIStatus *s) @@ -798,7 +800,8 @@ static CMPIInstance *get_resource_rasd(struct virt_pool_res *res, inst = get_typed_instance(_BROKER, CLASSNAME(ref), "StorageVolumeResourceAllocationSettingData", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, @@ -1279,7 +1282,8 @@ CMPIStatus get_rpcs(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "ResourcePoolConfigurationService", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index 9eb9e57..5091205 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -113,7 +113,8 @@ static CMPIInstance *default_vssd_instance(const char *prefix, inst = get_typed_instance(_BROKER, prefix, "VirtualSystemSettingData", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Failed to create default VSSD instance"); goto out; @@ -303,7 +304,8 @@ static CMPIInstance *sdc_rasd_inst(CMPIStatus *s, inst = get_typed_instance(_BROKER, CLASSNAME(ref), base, - NAMESPACE(ref)); + NAMESPACE(ref), + false);
if (inst == NULL) { cu_statusf(_BROKER, s, diff --git a/src/Virt_SwitchService.c b/src/Virt_SwitchService.c index 7e59d38..8991426 100644 --- a/src/Virt_SwitchService.c +++ b/src/Virt_SwitchService.c @@ -229,7 +229,8 @@ static CMPIStatus get_switchservice(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "SwitchService", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_VSMigrationCapabilities.c b/src/Virt_VSMigrationCapabilities.c index 4f0e434..3e53f68 100644 --- a/src/Virt_VSMigrationCapabilities.c +++ b/src/Virt_VSMigrationCapabilities.c @@ -134,7 +134,8 @@ CMPIStatus get_migration_caps(const CMPIObjectPath *ref, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemMigrationCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 76e3d25..1f6659d 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -831,7 +831,8 @@ static CMPIInstance *prepare_indication(const CMPIBroker *broker, ind = get_typed_instance(broker, pfx, ind_name, - job->ref_ns); + job->ref_ns, + false); if (ind == NULL) { CU_DEBUG("Failed to create ind, type '%s:%s_%s'", job->ref_ns, pfx, ind_name); @@ -1686,7 +1687,8 @@ CMPIStatus get_migration_service(const CMPIObjectPath *ref, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemMigrationService", - NAMESPACE(ref)); + NAMESPACE(ref), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSMigrationSettingData.c b/src/Virt_VSMigrationSettingData.c index a6707bd..eb545f1 100644 --- a/src/Virt_VSMigrationSettingData.c +++ b/src/Virt_VSMigrationSettingData.c @@ -79,7 +79,8 @@ CMPIStatus get_migration_sd(const CMPIObjectPath *ref, inst = get_typed_instance(broker, CLASSNAME(ref), "VirtualSystemMigrationSettingData", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 499b157..975623b 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -285,7 +285,8 @@ static CMPIInstance *_get_vssd(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSettingData", - NAMESPACE(reference)); + NAMESPACE(reference), + false);
if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_VirtualSystemManagementCapabilities.c b/src/Virt_VirtualSystemManagementCapabilities.c index 020ce8a..51738ee 100644 --- a/src/Virt_VirtualSystemManagementCapabilities.c +++ b/src/Virt_VirtualSystemManagementCapabilities.c @@ -129,7 +129,8 @@ CMPIStatus get_vsm_cap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemManagementCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 10adf8b..96c8a03 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -1928,7 +1928,8 @@ static CMPIStatus raise_rasd_indication(const CMPIContext *context, ind = get_typed_instance(_BROKER, CLASSNAME(ref), base_type, - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (ind == NULL) { CU_DEBUG("Failed to get indication instance"); s.rc = CMPI_RC_ERR_FAILED; @@ -3293,7 +3294,8 @@ CMPIStatus get_vsms(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemManagementService", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_VirtualSystemSnapshotService.c b/src/Virt_VirtualSystemSnapshotService.c index aae628f..8c0889d 100644 --- a/src/Virt_VirtualSystemSnapshotService.c +++ b/src/Virt_VirtualSystemSnapshotService.c @@ -681,7 +681,8 @@ CMPIStatus get_vsss(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSnapshotService", - NAMESPACE(ref)); + NAMESPACE(ref), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VirtualSystemSnapshotServiceCapabilities.c b/src/Virt_VirtualSystemSnapshotServiceCapabilities.c index 69c8e97..04a9c7a 100644 --- a/src/Virt_VirtualSystemSnapshotServiceCapabilities.c +++ b/src/Virt_VirtualSystemSnapshotServiceCapabilities.c @@ -117,7 +117,8 @@ CMPIStatus get_vss_cap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSnapshotServiceCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED,

于 2013-3-22 0:36, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
There are some instances which did not register this property, so this patch added a parameter in get_typed_instance(). The caller must set it clearly whether to set the property. When tog-pegasus > 2.12 and its log >= WARNING, tog-pegasus will report this problem in log.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libxkutil/misc_util.c | 12 ++++++++---- libxkutil/misc_util.h | 3 ++- src/Virt_AllocationCapabilities.c | 3 ++- src/Virt_ComputerSystem.c | 6 ++++-- src/Virt_ComputerSystemIndication.c | 3 ++- src/Virt_ConsoleRedirectionService.c | 3 ++- src/Virt_ConsoleRedirectionServiceCapabilities.c | 3 ++- src/Virt_Device.c | 18 ++++++++++++------ src/Virt_DevicePool.c | 20 +++++++++++++------- src/Virt_EnabledLogicalElementCapabilities.c | 3 ++- src/Virt_FilterEntry.c | 3 ++- src/Virt_FilterList.c | 3 ++- src/Virt_HostSystem.c | 3 ++- src/Virt_KVMRedirectionSAP.c | 3 ++- src/Virt_RASD.c | 3 ++- src/Virt_ReferencedProfile.c | 3 ++- src/Virt_RegisteredProfile.c | 3 ++- src/Virt_ResourcePoolConfigurationCapabilities.c | 3 ++- src/Virt_ResourcePoolConfigurationService.c | 8 ++++++-- src/Virt_SettingsDefineCapabilities.c | 6 ++++-- src/Virt_SwitchService.c | 3 ++- src/Virt_VSMigrationCapabilities.c | 3 ++- src/Virt_VSMigrationService.c | 6 ++++-- src/Virt_VSMigrationSettingData.c | 3 ++- src/Virt_VSSD.c | 3 ++- src/Virt_VirtualSystemManagementCapabilities.c | 3 ++- src/Virt_VirtualSystemManagementService.c | 6 ++++-- src/Virt_VirtualSystemSnapshotService.c | 3 ++- ...Virt_VirtualSystemSnapshotServiceCapabilities.c | 3 ++- 29 files changed, 97 insertions(+), 48 deletions(-)
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index 2d149ae..f1b93e4 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -341,7 +341,8 @@ char *get_typed_class(const char *refcn, const char *new_base) CMPIInstance *get_typed_instance(const CMPIBroker *broker, const char *refcn, const char *base, - const char *namespace) + const char *namespace, + bool ccn_flag)
While it may seem obvious to you why one would want to provide 'true' or 'false' here, could the function header elaborate a bit so that in the future when someone "adds" a call to this function they will know whether they should pass true or false. IOW which type of instances need this and which do not.
If the answer is "as according to the DSP" - I think that's fine. I don't know where to find the DSP (yet) or at least where/how to make the determination that the caller would (or not) need the property.
There is a standard for virtulization with CIM, whether setting it depends on that. http://dmtf.org/standards/vman the path is: DSP standard doc->cim schema file->libvirt-cim code. At least schema need to be consistent with libvirt-cim code, when not sure the standard doc can be checked.
From my scan of these - it sems *Capabilities, *Indications, *Filter, *Profiles, & *RASDs type classes don't need the property, while specific element classes (ComputerSystem, HostSystem, LogicalDisk, NetworkPort, etc) and *Service classes need the property. Is that a "fair" summary?
I think so.
ACK with description
the remainder looks fine.
John
{ char *new_cn; CMPIObjectPath *op; @@ -360,8 +361,10 @@ CMPIInstance *get_typed_instance(const CMPIBroker *broker, if ((s.rc != CMPI_RC_OK) || CMIsNullObject(inst)) goto out;
- CMSetProperty(inst, "CreationClassName", - (CMPIValue *)new_cn, CMPI_chars); + if (ccn_flag) { + CMSetProperty(inst, "CreationClassName", + (CMPIValue *)new_cn, CMPI_chars); + }
out: free(new_cn); @@ -467,7 +470,8 @@ CMPIInstance *make_reference(const CMPIBroker *broker, ref_inst = get_typed_instance(broker, CLASSNAME(source_ref), assoc_classname, - NAMESPACE(source_ref)); + NAMESPACE(source_ref), + false);
if (ref_inst != NULL) { CMPIObjectPath *target_ref; diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h index c7a2122..90fb2da 100644 --- a/libxkutil/misc_util.h +++ b/libxkutil/misc_util.h @@ -85,7 +85,8 @@ char *get_typed_class(const char *refcn, const char *new_base); CMPIInstance *get_typed_instance(const CMPIBroker *broker, const char *refcn, const char *base, - const char *namespace); + const char *namespace, + bool ccn_flag);
/* Parse an OrgID:LocID string into its constituent parts */ int parse_instance_id(char *iid, char **orgid, char **locid); diff --git a/src/Virt_AllocationCapabilities.c b/src/Virt_AllocationCapabilities.c index 970abeb..b358fac 100644 --- a/src/Virt_AllocationCapabilities.c +++ b/src/Virt_AllocationCapabilities.c @@ -46,7 +46,8 @@ static CMPIStatus ac_from_pool(const CMPIBroker *broker, *alloc_cap = get_typed_instance(broker, CLASSNAME(ref), "AllocationCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (*alloc_cap == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index e6c7e55..4a9b26d 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -440,7 +440,8 @@ CMPIStatus instance_from_dominfo(const CMPIBroker *broker, inst = get_typed_instance(broker, prefix, "ComputerSystem", - namespace); + namespace, + true);
if (inst == NULL) { CU_DEBUG("Could not init CS instance. " @@ -560,7 +561,8 @@ static CMPIStatus instance_from_dom(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "ComputerSystem", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 2d0a94e..3096683 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -239,7 +239,8 @@ static bool _do_indication(const CMPIBroker *broker, ind = get_typed_instance(broker, prefix, ind_type_name, - args->ns); + args->ns, + false);
/* Generally report errors and hope to continue, since we have no one to actually return status to. */ diff --git a/src/Virt_ConsoleRedirectionService.c b/src/Virt_ConsoleRedirectionService.c index 7b20c0d..16cade8 100644 --- a/src/Virt_ConsoleRedirectionService.c +++ b/src/Virt_ConsoleRedirectionService.c @@ -129,7 +129,8 @@ CMPIStatus get_console_rs(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "ConsoleRedirectionService", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_ConsoleRedirectionServiceCapabilities.c b/src/Virt_ConsoleRedirectionServiceCapabilities.c index 11f2986..88fb3a4 100644 --- a/src/Virt_ConsoleRedirectionServiceCapabilities.c +++ b/src/Virt_ConsoleRedirectionServiceCapabilities.c @@ -91,7 +91,8 @@ CMPIStatus get_console_rs_caps(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "ConsoleRedirectionServiceCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_Device.c b/src/Virt_Device.c index e047a94..c7dcbc3 100644 --- a/src/Virt_Device.c +++ b/src/Virt_Device.c @@ -100,7 +100,8 @@ static CMPIInstance *net_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "NetworkPort", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for NetworkPort"); @@ -138,7 +139,8 @@ static CMPIInstance *disk_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "LogicalDisk", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for LogicalDisk"); @@ -185,7 +187,8 @@ static CMPIInstance *mem_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "Memory", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for Memory"); @@ -234,7 +237,8 @@ static CMPIInstance *graphics_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "DisplayController", - ns); + ns, + true);
if (inst == NULL) { CU_DEBUG("Failed to get instance for DisplayController"); @@ -320,7 +324,8 @@ static CMPIInstance *input_instance(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "PointingDevice", - ns); + ns, + true); if (inst == NULL) { CU_DEBUG("Failed to get instance of %s_PointingDevice", pfx_from_conn(conn)); @@ -407,7 +412,8 @@ static bool vcpu_inst(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "Processor", - ns); + ns, + true); if (inst == NULL) return false;
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 202e509..56c9715 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -803,7 +803,8 @@ static CMPIStatus mempool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "MemoryPool", - ns); + ns, + false);
if (inst == NULL) { cu_statusf(broker, &s, @@ -842,7 +843,8 @@ static CMPIStatus procpool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "ProcessorPool", - ns); + ns, + false);
if (inst == NULL) { cu_statusf(broker, &s, @@ -872,7 +874,8 @@ static CMPIStatus _netpool_for_parent(struct inst_list *list, inst = get_typed_instance(broker, refcn, "NetworkPool", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Unable to get instance: %s:%s_NetworkPool", ns, refcn); @@ -928,7 +931,8 @@ static CMPIStatus _netpool_for_network(struct inst_list *list, inst = get_typed_instance(broker, refcn, "NetworkPool", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Unable to get instance: %s:%s_NetworkPool", ns, refcn); @@ -1046,7 +1050,7 @@ static CMPIInstance *diskpool_from_path(struct tmp_disk_pool *pool, CMPIInstance *inst; char *poolid = NULL;
- inst = get_typed_instance(broker, refcn, "DiskPool", ns); + inst = get_typed_instance(broker, refcn, "DiskPool", ns, false);
if (inst == NULL) { cu_statusf(broker, &s, @@ -1136,7 +1140,8 @@ static CMPIStatus graphicspool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "GraphicsPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, @@ -1172,7 +1177,8 @@ static CMPIStatus inputpool_instance(virConnectPtr conn, inst = get_typed_instance(broker, pfx_from_conn(conn), "InputPool", - ns); + ns, + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_EnabledLogicalElementCapabilities.c b/src/Virt_EnabledLogicalElementCapabilities.c index 7ba5eae..2fcdef1 100644 --- a/src/Virt_EnabledLogicalElementCapabilities.c +++ b/src/Virt_EnabledLogicalElementCapabilities.c @@ -61,7 +61,8 @@ static CMPIInstance *_get_elec(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "EnabledLogicalElementCapabilities", - NAMESPACE(reference)); + NAMESPACE(reference), + false); if (inst == NULL) { cu_statusf(broker, s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_FilterEntry.c b/src/Virt_FilterEntry.c index 126615b..3c4a3e6 100644 --- a/src/Virt_FilterEntry.c +++ b/src/Virt_FilterEntry.c @@ -546,7 +546,8 @@ static CMPIInstance *convert_rule_to_instance( inst = get_typed_instance(broker, CLASSNAME(reference), basename, - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_FilterList.c b/src/Virt_FilterList.c index 9b5dbae..79776cd 100644 --- a/src/Virt_FilterList.c +++ b/src/Virt_FilterList.c @@ -50,7 +50,8 @@ static CMPIInstance *convert_filter_to_instance( inst = get_typed_instance(broker, CLASSNAME(reference), "FilterList", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_HostSystem.c b/src/Virt_HostSystem.c index 724a5ea..c31d6cf 100644 --- a/src/Virt_HostSystem.c +++ b/src/Virt_HostSystem.c @@ -135,7 +135,8 @@ static CMPIStatus fake_host(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "HostSystem", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { cu_statusf(broker, &s, diff --git a/src/Virt_KVMRedirectionSAP.c b/src/Virt_KVMRedirectionSAP.c index db34d57..38ad468 100644 --- a/src/Virt_KVMRedirectionSAP.c +++ b/src/Virt_KVMRedirectionSAP.c @@ -125,7 +125,8 @@ static CMPIInstance *get_console_sap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "KVMRedirectionSAP", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c index 9493077..6e8a244 100644 --- a/src/Virt_RASD.c +++ b/src/Virt_RASD.c @@ -716,7 +716,8 @@ CMPIInstance *rasd_from_vdev(const CMPIBroker *broker, inst = get_typed_instance(broker, CLASSNAME(ref), base, - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) return inst;
diff --git a/src/Virt_ReferencedProfile.c b/src/Virt_ReferencedProfile.c index 150be1f..e78a8d3 100644 --- a/src/Virt_ReferencedProfile.c +++ b/src/Virt_ReferencedProfile.c @@ -242,7 +242,8 @@ static CMPIInstance *make_ref(const CMPIObjectPath *source_ref, ref_inst = get_typed_instance(_BROKER, CLASSNAME(source_ref), assoc_classname, - NAMESPACE(source_ref)); + NAMESPACE(source_ref), + false);
source = get_reg_prof_by_ref(source_ref); if (source->scoping_profile != NULL) diff --git a/src/Virt_RegisteredProfile.c b/src/Virt_RegisteredProfile.c index 389a179..e644708 100644 --- a/src/Virt_RegisteredProfile.c +++ b/src/Virt_RegisteredProfile.c @@ -54,7 +54,8 @@ CMPIStatus get_profile(const CMPIBroker *broker, instance = get_typed_instance(broker, pfx, "RegisteredProfile", - CIM_INTEROP_NS); + CIM_INTEROP_NS, + false);
if (instance == NULL) { cu_statusf(broker, &s, diff --git a/src/Virt_ResourcePoolConfigurationCapabilities.c b/src/Virt_ResourcePoolConfigurationCapabilities.c index 32274ed..2dcbbcf 100644 --- a/src/Virt_ResourcePoolConfigurationCapabilities.c +++ b/src/Virt_ResourcePoolConfigurationCapabilities.c @@ -69,7 +69,8 @@ static CMPIStatus get_rpc_cap(const CMPIObjectPath *reference, inst = get_typed_instance(_BROKER, pfx_from_conn(conn), "ResourcePoolConfigurationCapabilities", - NAMESPACE(reference)); + NAMESPACE(reference), + false); if (inst == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_ResourcePoolConfigurationService.c b/src/Virt_ResourcePoolConfigurationService.c index 751d016..0c0cc06 100644 --- a/src/Virt_ResourcePoolConfigurationService.c +++ b/src/Virt_ResourcePoolConfigurationService.c @@ -781,6 +781,8 @@ static const char *rasd_to_res(CMPIInstance *inst, return msg; }
+/* Warning: returned instance is not freed manually in caller, need confirm + if server will auto free it. */ static CMPIInstance *get_resource_rasd(struct virt_pool_res *res, const CMPIObjectPath *ref, CMPIStatus *s) @@ -798,7 +800,8 @@ static CMPIInstance *get_resource_rasd(struct virt_pool_res *res, inst = get_typed_instance(_BROKER, CLASSNAME(ref), "StorageVolumeResourceAllocationSettingData", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(_BROKER, s, CMPI_RC_ERR_FAILED, @@ -1279,7 +1282,8 @@ CMPIStatus get_rpcs(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "ResourcePoolConfigurationService", - NAMESPACE(reference)); + NAMESPACE(reference), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index 9eb9e57..5091205 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -113,7 +113,8 @@ static CMPIInstance *default_vssd_instance(const char *prefix, inst = get_typed_instance(_BROKER, prefix, "VirtualSystemSettingData", - ns); + ns, + false); if (inst == NULL) { CU_DEBUG("Failed to create default VSSD instance"); goto out; @@ -303,7 +304,8 @@ static CMPIInstance *sdc_rasd_inst(CMPIStatus *s, inst = get_typed_instance(_BROKER, CLASSNAME(ref), base, - NAMESPACE(ref)); + NAMESPACE(ref), + false);
if (inst == NULL) { cu_statusf(_BROKER, s, diff --git a/src/Virt_SwitchService.c b/src/Virt_SwitchService.c index 7e59d38..8991426 100644 --- a/src/Virt_SwitchService.c +++ b/src/Virt_SwitchService.c @@ -229,7 +229,8 @@ static CMPIStatus get_switchservice(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "SwitchService", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_VSMigrationCapabilities.c b/src/Virt_VSMigrationCapabilities.c index 4f0e434..3e53f68 100644 --- a/src/Virt_VSMigrationCapabilities.c +++ b/src/Virt_VSMigrationCapabilities.c @@ -134,7 +134,8 @@ CMPIStatus get_migration_caps(const CMPIObjectPath *ref, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemMigrationCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 76e3d25..1f6659d 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -831,7 +831,8 @@ static CMPIInstance *prepare_indication(const CMPIBroker *broker, ind = get_typed_instance(broker, pfx, ind_name, - job->ref_ns); + job->ref_ns, + false); if (ind == NULL) { CU_DEBUG("Failed to create ind, type '%s:%s_%s'", job->ref_ns, pfx, ind_name); @@ -1686,7 +1687,8 @@ CMPIStatus get_migration_service(const CMPIObjectPath *ref, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemMigrationService", - NAMESPACE(ref)); + NAMESPACE(ref), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSMigrationSettingData.c b/src/Virt_VSMigrationSettingData.c index a6707bd..eb545f1 100644 --- a/src/Virt_VSMigrationSettingData.c +++ b/src/Virt_VSMigrationSettingData.c @@ -79,7 +79,8 @@ CMPIStatus get_migration_sd(const CMPIObjectPath *ref, inst = get_typed_instance(broker, CLASSNAME(ref), "VirtualSystemMigrationSettingData", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 499b157..975623b 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -285,7 +285,8 @@ static CMPIInstance *_get_vssd(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSettingData", - NAMESPACE(reference)); + NAMESPACE(reference), + false);
if (inst == NULL) { cu_statusf(broker, s, diff --git a/src/Virt_VirtualSystemManagementCapabilities.c b/src/Virt_VirtualSystemManagementCapabilities.c index 020ce8a..51738ee 100644 --- a/src/Virt_VirtualSystemManagementCapabilities.c +++ b/src/Virt_VirtualSystemManagementCapabilities.c @@ -129,7 +129,8 @@ CMPIStatus get_vsm_cap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemManagementCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 10adf8b..96c8a03 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -1928,7 +1928,8 @@ static CMPIStatus raise_rasd_indication(const CMPIContext *context, ind = get_typed_instance(_BROKER, CLASSNAME(ref), base_type, - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (ind == NULL) { CU_DEBUG("Failed to get indication instance"); s.rc = CMPI_RC_ERR_FAILED; @@ -3293,7 +3294,8 @@ CMPIStatus get_vsms(const CMPIObjectPath *reference, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemManagementService", - NAMESPACE(reference)); + NAMESPACE(reference), + true);
if (inst == NULL) { CU_DEBUG("Failed to get typed instance"); diff --git a/src/Virt_VirtualSystemSnapshotService.c b/src/Virt_VirtualSystemSnapshotService.c index aae628f..8c0889d 100644 --- a/src/Virt_VirtualSystemSnapshotService.c +++ b/src/Virt_VirtualSystemSnapshotService.c @@ -681,7 +681,8 @@ CMPIStatus get_vsss(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSnapshotService", - NAMESPACE(ref)); + NAMESPACE(ref), + true); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED, diff --git a/src/Virt_VirtualSystemSnapshotServiceCapabilities.c b/src/Virt_VirtualSystemSnapshotServiceCapabilities.c index 69c8e97..04a9c7a 100644 --- a/src/Virt_VirtualSystemSnapshotServiceCapabilities.c +++ b/src/Virt_VirtualSystemSnapshotServiceCapabilities.c @@ -117,7 +117,8 @@ CMPIStatus get_vss_cap(const CMPIBroker *broker, inst = get_typed_instance(broker, pfx_from_conn(conn), "VirtualSystemSnapshotServiceCapabilities", - NAMESPACE(ref)); + NAMESPACE(ref), + false); if (inst == NULL) { cu_statusf(broker, &s, CMPI_RC_ERR_FAILED,
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

This property is used but not defined, so add it. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- schema/Virt_VSSD.mof | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/schema/Virt_VSSD.mof b/schema/Virt_VSSD.mof index baf4ac2..b960450 100644 --- a/schema/Virt_VSSD.mof +++ b/schema/Virt_VSSD.mof @@ -27,4 +27,7 @@ class Virt_VirtualSystemSettingData : CIM_VirtualSystemSettingData [Description ("Flag to determine whether this guest has to be autostarted on reboot")] uint16 AutoStart; + [Description ("Flag to determine whether this guest is a full virtualization")] + boolean IsFullVirt; + }; -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This property is used but not defined, so add it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- schema/Virt_VSSD.mof | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/schema/Virt_VSSD.mof b/schema/Virt_VSSD.mof index baf4ac2..b960450 100644 --- a/schema/Virt_VSSD.mof +++ b/schema/Virt_VSSD.mof @@ -27,4 +27,7 @@ class Virt_VirtualSystemSettingData : CIM_VirtualSystemSettingData [Description ("Flag to determine whether this guest has to be autostarted on reboot")] uint16 AutoStart;
+ [Description ("Flag to determine whether this guest is a full virtualization")] + boolean IsFullVirt; + };
This property is found in 'VSSD.mof': class Xen_VirtualSystemSettingData : Virt_VirtualSystemSettingData { [Description ("Flag to determine whether this guest is fully-virtualized")] boolean IsFullVirt; ... Since that is a child of the one you are placing the the property into, shouldn't it be removed from the child? Also I think the Description in VSSD.mof is better than the one in Virt_VSSD.mof. John

于 2013-3-22 0:51, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This property is used but not defined, so add it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- schema/Virt_VSSD.mof | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/schema/Virt_VSSD.mof b/schema/Virt_VSSD.mof index baf4ac2..b960450 100644 --- a/schema/Virt_VSSD.mof +++ b/schema/Virt_VSSD.mof @@ -27,4 +27,7 @@ class Virt_VirtualSystemSettingData : CIM_VirtualSystemSettingData [Description ("Flag to determine whether this guest has to be autostarted on reboot")] uint16 AutoStart;
+ [Description ("Flag to determine whether this guest is a full virtualization")] + boolean IsFullVirt; + };
This property is found in 'VSSD.mof':
class Xen_VirtualSystemSettingData : Virt_VirtualSystemSettingData {
[Description ("Flag to determine whether this guest is fully-virtualized")] boolean IsFullVirt; ...
Since that is a child of the one you are placing the the property into, shouldn't it be removed from the child?
I found this property is used for KVM/Xen, no LXC, so I guess adding it in KVM_VSSD in VSSD.mof is a better way. I'll change it in next version. Note there is a bug like code in vssd_to_domain(), which treat this property as a condition before entering child class property retrieving. It is why I added it in parent class before.
Also I think the Description in VSSD.mof is better than the one in Virt_VSSD.mof.
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This property is used but not defined, so add it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- schema/Virt_VSSD.mof | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/schema/Virt_VSSD.mof b/schema/Virt_VSSD.mof index baf4ac2..b960450 100644 --- a/schema/Virt_VSSD.mof +++ b/schema/Virt_VSSD.mof @@ -27,4 +27,7 @@ class Virt_VirtualSystemSettingData : CIM_VirtualSystemSettingData [Description ("Flag to determine whether this guest has to be autostarted on reboot")] uint16 AutoStart;
+ [Description ("Flag to determine whether this guest is a full virtualization")] + boolean IsFullVirt; + };
Follow up note... Is this "isFullVirt" or "IsFullVirt" - I see it used both ways, so I'm confused. John

于 2013-3-22 0:56, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This property is used but not defined, so add it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- schema/Virt_VSSD.mof | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/schema/Virt_VSSD.mof b/schema/Virt_VSSD.mof index baf4ac2..b960450 100644 --- a/schema/Virt_VSSD.mof +++ b/schema/Virt_VSSD.mof @@ -27,4 +27,7 @@ class Virt_VirtualSystemSettingData : CIM_VirtualSystemSettingData [Description ("Flag to determine whether this guest has to be autostarted on reboot")] uint16 AutoStart;
+ [Description ("Flag to determine whether this guest is a full virtualization")] + boolean IsFullVirt; + };
Follow up note... Is this "isFullVirt" or "IsFullVirt" - I see it used both ways, so I'm confused.
It should be later one, in code it must be miss spelled.
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

The property registerted is BootDevices, so correct it. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_SettingsDefineCapabilities.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index 5091205..1e7a778 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -172,7 +172,7 @@ static CMPIStatus _xen_vsmc_to_vssd(virConnectPtr conn, if (inst == NULL) goto error; - CMSetProperty(inst, "BootDevice", + CMSetProperty(inst, "BootDevices", (CMPIValue *)"hda", CMPI_chars); CMSetProperty(inst, "isFullVirt", @@ -211,7 +211,7 @@ static CMPIStatus _kvm_vsmc_to_vssd(virConnectPtr conn, CMSetProperty(inst, "VirtualSystemIdentifier", (CMPIValue *)"KVM_guest", CMPI_chars); - CMSetProperty(inst, "BootDevice", + CMSetProperty(inst, "BootDevices", (CMPIValue *)"hda", CMPI_chars); inst_list_add(list, inst); -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
The property registerted is BootDevices, so correct it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_SettingsDefineCapabilities.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index 5091205..1e7a778 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -172,7 +172,7 @@ static CMPIStatus _xen_vsmc_to_vssd(virConnectPtr conn, if (inst == NULL) goto error;
- CMSetProperty(inst, "BootDevice", + CMSetProperty(inst, "BootDevices", (CMPIValue *)"hda", CMPI_chars);
CMSetProperty(inst, "isFullVirt", @@ -211,7 +211,7 @@ static CMPIStatus _kvm_vsmc_to_vssd(virConnectPtr conn, CMSetProperty(inst, "VirtualSystemIdentifier", (CMPIValue *)"KVM_guest", CMPI_chars);
- CMSetProperty(inst, "BootDevice", + CMSetProperty(inst, "BootDevices", (CMPIValue *)"hda", CMPI_chars);
inst_list_add(list, inst);
ACK Note: In this module, the "isFullVirt" is used - hence my follow-up question to patch 2/15. I also found two instances where "BootDevice" is used in CU_DEBUG messages (not that it matters, but it's a consistency thing for me). John

于 2013-3-22 1:00, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
The property registerted is BootDevices, so correct it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_SettingsDefineCapabilities.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c index 5091205..1e7a778 100644 --- a/src/Virt_SettingsDefineCapabilities.c +++ b/src/Virt_SettingsDefineCapabilities.c @@ -172,7 +172,7 @@ static CMPIStatus _xen_vsmc_to_vssd(virConnectPtr conn, if (inst == NULL) goto error;
- CMSetProperty(inst, "BootDevice", + CMSetProperty(inst, "BootDevices", (CMPIValue *)"hda", CMPI_chars);
CMSetProperty(inst, "isFullVirt", @@ -211,7 +211,7 @@ static CMPIStatus _kvm_vsmc_to_vssd(virConnectPtr conn, CMSetProperty(inst, "VirtualSystemIdentifier", (CMPIValue *)"KVM_guest", CMPI_chars);
- CMSetProperty(inst, "BootDevice", + CMSetProperty(inst, "BootDevices", (CMPIValue *)"hda", CMPI_chars);
inst_list_add(list, inst);
ACK
Note: In this module, the "isFullVirt" is used - hence my follow-up question to patch 2/15.
It should be a misspelling, I'll fix it also in next version.
I also found two instances where "BootDevice" is used in CU_DEBUG messages (not that it matters, but it's a consistency thing for me).
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

Now yum upgrade will deregister the virt classes, make libvirt-cim not workable. This patch fixed it from now on. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libvirt-cim.spec.in | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/libvirt-cim.spec.in b/libvirt-cim.spec.in index d78eee7..3def978 100644 --- a/libvirt-cim.spec.in +++ b/libvirt-cim.spec.in @@ -104,18 +104,22 @@ rm -fr $RPM_BUILD_ROOT -r %{CIMV2_REG} -m %{CIMV2_MOF} -v >/dev/null 2>&1 || true %preun -%{_datadir}/%{name}/provider-register.sh -d -t pegasus \ +#Deregister only in uninstall, do nothing in upgrade. +if [ "$1" = "0" ]; then + echo "Deleting registered classes in libvirt-cim..." + %{_datadir}/%{name}/provider-register.sh -d -t pegasus \ -n @CIM_VIRT_NS@ \ -r %{REGISTRATION} -m %{SCHEMA} >/dev/null 2>&1 || true -%{_datadir}/%{name}/provider-register.sh -d -t pegasus \ + %{_datadir}/%{name}/provider-register.sh -d -t pegasus \ -n root/interop \ -r %{INTEROP_REG} -m %{INTEROP_MOF} >/dev/null 2>&1 || true -%{_datadir}/%{name}/provider-register.sh -d -t pegasus \ + %{_datadir}/%{name}/provider-register.sh -d -t pegasus \ -n root/PG_InterOp \ -r %{PGINTEROP_REG} -m %{PGINTEROP_MOF} >/dev/null 2>&1 || true -%{_datadir}/%{name}/provider-register.sh -d -t pegasus \ + %{_datadir}/%{name}/provider-register.sh -d -t pegasus \ -n root/cimv2 \ -r %{CIMV2_REG} -m %{CIMV2_MOF} >/dev/null 2>&1 || true +fi %postun -p /sbin/ldconfig -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Now yum upgrade will deregister the virt classes, make libvirt-cim not workable. This patch fixed it from now on.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libvirt-cim.spec.in | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
ACK John

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Sharad Mishra <snmishra@us.ibm.com> --- src/Virt_ComputerSystemIndication.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 3096683..8250850 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -504,7 +504,7 @@ static void csi_domain_event_cb(virConnectPtr conn, CMPIStatus s = {CMPI_RC_OK, NULL}; if (lifecycle_enabled == false || thread->active_filters <= 0) { - CU_DEBUG("%s indications deactivated, return"); + CU_DEBUG("%s indications deactivated, return", prefix); return; } -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Sharad Mishra <snmishra@us.ibm.com> --- src/Virt_ComputerSystemIndication.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK John

This patch use lock 'lifecycle_mutex' to protect gloable shared data in lifecycle_thread(). This lock exist in Activate/Deactivate Enable/Disable method, which is meant to protect the gloable shared data, but forgot to be added in new libvirt based CSI thread. This patch can avoid following risk at least: Original code have a small chance to free thread->args in child thread just after main thread malloc it, for that thread->id is set to zero allowing main thread to enter that code. This patch focus on adding missing lock, the CSI can be still improved as: smaller lock, folder lock into a structure with data to tip better what it is doing. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 39 ++++++++++++++++++++++++++++++++-- 1 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 8250850..3df47fa 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -174,9 +174,11 @@ static void csi_free_thread_data(void *data) if (data == NULL) return; + pthread_mutex_lock(&lifecycle_mutex); list_free(thread->dom_list); thread->dom_list = NULL; stdi_free_ind_args(&thread->args); + pthread_mutex_unlock(&lifecycle_mutex); } void set_source_inst_props(const CMPIBroker *broker, @@ -491,6 +493,8 @@ static int update_domain_list(virConnectPtr conn, csi_thread_data_t *thread) return s.rc; } +/* following function will protect global data with lifecycle_mutex. + TODO: improve it with seperate lock later. */ static void csi_domain_event_cb(virConnectPtr conn, virDomainPtr dom, int event, @@ -503,10 +507,13 @@ static void csi_domain_event_cb(virConnectPtr conn, char *prefix = class_prefix_name(thread->args->classname); CMPIStatus s = {CMPI_RC_OK, NULL}; + pthread_mutex_lock(&lifecycle_mutex); if (lifecycle_enabled == false || thread->active_filters <= 0) { CU_DEBUG("%s indications deactivated, return", prefix); + pthread_mutex_unlock(&lifecycle_mutex); return; } + pthread_mutex_unlock(&lifecycle_mutex); CU_DEBUG("Event: Domain %s(%d) event: %d detail: %d\n", virDomainGetName(dom), virDomainGetID(dom), event, detail); @@ -538,7 +545,9 @@ static void csi_domain_event_cb(virConnectPtr conn, if (cs_event != CS_CREATED) { char uuid[VIR_UUID_STRING_BUFLEN] = {0}; virDomainGetUUIDString(dom, &uuid[0]); + pthread_mutex_lock(&lifecycle_mutex); dom_xml = list_find(thread->dom_list, uuid); + pthread_mutex_unlock(&lifecycle_mutex); } if (dom_xml == NULL) { @@ -546,12 +555,16 @@ static void csi_domain_event_cb(virConnectPtr conn, goto end; } + pthread_mutex_lock(&lifecycle_mutex); async_ind(thread->args, cs_event, dom_xml, prefix); + pthread_mutex_unlock(&lifecycle_mutex); /* Update the domain list accordingly */ if (event == VIR_DOMAIN_EVENT_DEFINED) { if (detail == VIR_DOMAIN_EVENT_DEFINED_ADDED) { + pthread_mutex_lock(&lifecycle_mutex); csi_thread_dom_list_append(thread, dom_xml); + pthread_mutex_unlock(&lifecycle_mutex); } else if (detail == VIR_DOMAIN_EVENT_DEFINED_UPDATED) { free(dom_xml->name); free(dom_xml->xml); @@ -559,7 +572,9 @@ static void csi_domain_event_cb(virConnectPtr conn, } } else if (event == VIR_DOMAIN_EVENT_DEFINED && detail == VIR_DOMAIN_EVENT_UNDEFINED_REMOVED) { + pthread_mutex_lock(&lifecycle_mutex); list_remove(thread->dom_list, dom_xml); + pthread_mutex_unlock(&lifecycle_mutex); } end: @@ -580,10 +595,12 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) if (prefix == NULL) goto init_out; + pthread_mutex_lock(&lifecycle_mutex); conn = connect_by_classname(_BROKER, args->classname, &s); if (conn == NULL) { CU_DEBUG("Unable to start lifecycle thread: " "Failed to connect (cn: %s)", args->classname); + pthread_mutex_unlock(&lifecycle_mutex); goto conn_out; } @@ -595,33 +612,47 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) if (cb_id == -1) { CU_DEBUG("Failed to register domain event watch for '%s'", - args->classname) + args->classname); + pthread_mutex_unlock(&lifecycle_mutex); goto cb_out; } CBAttachThread(_BROKER, args->context); /* Get currently defined domains */ - if (update_domain_list(conn, thread) != CMPI_RC_OK) + if (update_domain_list(conn, thread) != CMPI_RC_OK) { + pthread_mutex_unlock(&lifecycle_mutex); goto end; + } + pthread_mutex_unlock(&lifecycle_mutex); CU_DEBUG("Entering CSI event loop (%s)", prefix); - while (thread->active_filters > 0) { + while (1) { + pthread_mutex_lock(&lifecycle_mutex); + if (thread->active_filters <= 0) { + pthread_mutex_unlock(&lifecycle_mutex); + break; + } + pthread_mutex_unlock(&lifecycle_mutex); if (virEventRunDefaultImpl() < 0) { virErrorPtr err = virGetLastError(); CU_DEBUG("Failed to run event loop: %s\n", err && err->message ? err->message : "Unknown error"); } + usleep(1); } CU_DEBUG("Exiting CSI event loop (%s)", prefix); + pthread_mutex_lock(&lifecycle_mutex); CBDetachThread(_BROKER, args->context); + pthread_mutex_unlock(&lifecycle_mutex); end: virConnectDomainEventDeregisterAny(conn, cb_id); cb_out: + pthread_mutex_lock(&lifecycle_mutex); thread->id = 0; thread->active_filters = 0; @@ -629,6 +660,8 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) if (thread->args != NULL) stdi_free_ind_args(&thread->args); + pthread_mutex_unlock(&lifecycle_mutex); + conn_out: virConnectClose(conn); -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This patch use lock 'lifecycle_mutex' to protect gloable shared data in lifecycle_thread(). This lock exist in Activate/Deactivate Enable/Disable method, which is meant to protect the gloable
s/gloable/global
shared data, but forgot to be added in new libvirt based CSI thread. This patch can avoid following risk at least: Original code have a small chance to free thread->args in child thread just after main thread malloc it, for that thread->id is set to zero allowing main thread to enter that code. This patch focus on adding missing lock, the CSI can be still improved as: smaller lock, folder lock into a structure with data to tip better what it is doing.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 39 ++++++++++++++++++++++++++++++++-- 1 files changed, 36 insertions(+), 3 deletions(-)
ACK John

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/Virt_DevicePool.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 56c9715..08677e2 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -710,7 +710,7 @@ static bool mempool_set_consumed(CMPIInstance *inst, virConnectPtr conn) dom = virDomainLookupByID(conn, domain_ids[i]); if (dom == NULL) { - CU_DEBUG("Cannot connect to domain %n: excluding", + CU_DEBUG("Cannot connect to domain %d: excluding", domain_ids[i]); continue; } -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/Virt_DevicePool.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK Although "theoretically speaking" this could/should be combined with 5/15 and 9/15 since they are similar (eg, via git rebase -i and squash 3 into 1 patch). John

于 2013-3-22 1:18, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- src/Virt_DevicePool.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK
Although "theoretically speaking" this could/should be combined with 5/15 and 9/15 since they are similar (eg, via git rebase -i and squash 3 into 1 patch).
John
OK, will squash them.
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

Original implemetion may return pools with NULL name if some pool disappear between two libvirt pool API call. And originally it return the number of pools and negative value when error happens, but caller of this function consider number = 0 as error. As a fix, this patch changed the function prototype, it do not return the pool number anymore, it returns 0 on success and negative on fail now. Code for checking the risk of returning pools with NULL name is also added. Another small fix is, return false in get_disk_parent() when strdup fail. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 176 +++++++++++++++++++++++++++++++++---------------- 1 files changed, 120 insertions(+), 56 deletions(-) diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 08677e2..185e3cc 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -51,6 +51,22 @@ struct tmp_disk_pool { bool primordial; }; +static void free_diskpool(struct tmp_disk_pool *pools, int count) +{ + int i; + + if (pools == NULL) { + return; + } + + for (i = 0; i < count; i++) { + free(pools[i].tag); + free(pools[i].path); + } + + free(pools); +} + /* * Right now, detect support and use it, if available. * Later, this can be a configure option if needed @@ -78,6 +94,10 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools, } pools[count].tag = strdup("0"); + if (pools[count].tag == NULL) { + count++; + goto free; + } pools[count].path = NULL; pools[count].primordial = true; count++; @@ -85,12 +105,17 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools, *_count = count; *_pools = pools; ret = true; + goto out; + free: + free_diskpool(pools, count); + /* old pool is invalid, update it */ + *_count = 0; + *_pools = NULL; out: return ret; } - #if VIR_USE_LIBVIRT_STORAGE int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) { @@ -117,52 +142,82 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) return ret; } +/* This function returns 0 on sucess, negative on fail. */ static int get_diskpool_config(virConnectPtr conn, - struct tmp_disk_pool **_pools) + struct tmp_disk_pool **_pools, + int *_count) { - int count = 0; + int count = 0, realcount = 0; int i; char ** names = NULL; struct tmp_disk_pool *pools = NULL; + int ret = 0; + bool bret; count = virConnectNumOfStoragePools(conn); - if (count <= 0) + if (count < 0) { + ret = count; goto out; + } else if (count == 0) { + goto set_parent; + } names = calloc(count, sizeof(char *)); if (names == NULL) { CU_DEBUG("Failed to alloc space for %i pool names", count); - count = 0; + ret = -1; goto out; } - if (virConnectListStoragePools(conn, names, count) == -1) { - CU_DEBUG("Failed to get storage pools"); - count = 0; - goto out; + realcount = virConnectListStoragePools(conn, names, count); + if (realcount < 0) { + CU_DEBUG("Failed to get storage pools, return %d.", realcount); + ret = realcount; + goto free_names; + } + if (realcount == 0) { + CU_DEBUG("Zero pools got, but prelist is %d.", count); + goto set_parent; } - pools = calloc(count, sizeof(*pools)); + pools = calloc(realcount, sizeof(*pools)); if (pools == NULL) { - CU_DEBUG("Failed to alloc space for %i pool structs", count); - goto out; + CU_DEBUG("Failed to alloc space for %i pool structs", + realcount); + ret = -2; + goto free_names; } - for (i = 0; i < count; i++) { - pools[i].tag = strdup(names[i]); + for (i = 0; i < realcount; i++) { + pools[i].tag = names[i]; + names[i] = NULL; pools[i].primordial = false; } - out: - for (i = 0; i < count; i++) - free(names[i]); - free(names); - - get_disk_parent(&pools, &count); + set_parent: + bret = get_disk_parent(&pools, &realcount); + if (bret != true) { + CU_DEBUG("Failed in adding parentpool."); + ret = -4; + goto free_pools; + } + /* succeed */ *_pools = pools; + *_count = realcount; + goto free_names; + + free_pools: + free_diskpool(pools, realcount); - return count; + free_names: + for (i = 0; i < count; i++) { + free(names[i]); + } + free(names); + + out: + return ret; } static bool diskpool_set_capacity(virConnectPtr conn, @@ -294,42 +349,59 @@ static int parse_diskpool_line(struct tmp_disk_pool *pool, return (ret == 2); } +/* return 0 on sucess, negative on fail. */ static int get_diskpool_config(virConnectPtr conn, - struct tmp_disk_pool **_pools) + struct tmp_disk_pool **_pools, + int *_count) { const char *path = DISK_POOL_CONFIG; FILE *config; char *line = NULL; size_t len = 0; - int count = 0; - struct tmp_disk_pool *pools = NULL; + int count = 0, ret = 0; + struct tmp_disk_pool *pools = NULL, *new_pools = NULL; + bool bret; config = fopen(path, "r"); if (config == NULL) { CU_DEBUG("Failed to open %s: %m", path); - return 0; + ret = -1; + goto out; } while (getline(&line, &len, config) > 0) { - pools = realloc(pools, - (count + 1) * (sizeof(*pools))); - if (pools == NULL) { + new_pools = realloc(pools, + (count + 1) * (sizeof(*pools))); + if (new_pools == NULL) { CU_DEBUG("Failed to alloc new pool"); - goto out; + ret = -2; + goto free_pools; } + pools = new_pools; if (parse_diskpool_line(&pools[count], line)) count++; } + bret = get_disk_parent(&pools, &count); + if (bret != true) { + CU_DEBUG("Failed in adding parentpool."); + ret = -3; + goto free_pools; + } - get_disk_parent(&pools, &count); - out: - free(line); + /* succeed */ *_pools = pools; - fclose(config); + *_count = count; + goto clean; - return count; + free_pools: + free_diskpool(pools, count); + clean: + free(line); + fclose(config); + out: + return ret; } static bool diskpool_set_capacity(virConnectPtr conn, @@ -367,39 +439,23 @@ static bool diskpool_set_capacity(virConnectPtr conn, } static bool _diskpool_is_member(virConnectPtr conn, - const struct disk_pool *pool, + const struct tmp_disk_pool *pool, const char *file) { return STARTS_WITH(file, pool->path); } #endif -static void free_diskpool(struct tmp_disk_pool *pools, int count) -{ - int i; - - if (pools == NULL) - return; - - for (i = 0; i < count; i++) { - free(pools[i].tag); - free(pools[i].path); - } - - free(pools); -} - static char *_diskpool_member_of(virConnectPtr conn, const char *file) { struct tmp_disk_pool *pools = NULL; int count; - int i; + int i, ret; char *pool = NULL; - count = get_diskpool_config(conn, &pools); - if (count == 0) { - free(pools); + ret = get_diskpool_config(conn, &pools, &count); + if (ret < 0) { return NULL; } @@ -1088,9 +1144,17 @@ static CMPIStatus diskpool_instance(virConnectPtr conn, CMPIStatus s = {CMPI_RC_OK, NULL}; struct tmp_disk_pool *pools = NULL; int count = 0; - int i; + int i, ret; - count = get_diskpool_config(conn, &pools); + ret = get_diskpool_config(conn, &pools, &count); + if (ret < 0) { + CU_DEBUG("Failed to get diskpool config, return is %d.", ret); + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed to get diskpool config, return is %d.", + ret); + return s; + } if ((id == NULL) && (count == 0)) { CU_DEBUG("No defined DiskPools"); free(pools); -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Original implemetion may return pools with NULL name if some pool disappear between two libvirt pool API call. And originally it return the number of pools and negative value when error happens, but caller of this function consider number = 0 as error. As a fix, this patch changed the function prototype, it do not return the pool number anymore, it returns 0 on success and negative on fail now. Code for checking the risk of returning pools with NULL name is also added. Another small fix is, return false in get_disk_parent() when strdup fail.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 176 +++++++++++++++++++++++++++++++++---------------- 1 files changed, 120 insertions(+), 56 deletions(-)
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 08677e2..185e3cc 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -51,6 +51,22 @@ struct tmp_disk_pool { bool primordial; };
+static void free_diskpool(struct tmp_disk_pool *pools, int count) +{ + int i; + + if (pools == NULL) { + return; + } + + for (i = 0; i < count; i++) { + free(pools[i].tag); + free(pools[i].path); + } + + free(pools); +} + /* * Right now, detect support and use it, if available. * Later, this can be a configure option if needed @@ -78,6 +94,10 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools, }
pools[count].tag = strdup("0"); + if (pools[count].tag == NULL) { + count++; + goto free; + } pools[count].path = NULL; pools[count].primordial = true; count++; @@ -85,12 +105,17 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools, *_count = count; *_pools = pools; ret = true; + goto out;
+ free: + free_diskpool(pools, count); + /* old pool is invalid, update it */ + *_count = 0; + *_pools = NULL; out: return ret; }
- #if VIR_USE_LIBVIRT_STORAGE int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) { @@ -117,52 +142,82 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) return ret; }
+/* This function returns 0 on sucess, negative on fail. */ static int get_diskpool_config(virConnectPtr conn, - struct tmp_disk_pool **_pools) + struct tmp_disk_pool **_pools, + int *_count) { - int count = 0; + int count = 0, realcount = 0; int i; char ** names = NULL; struct tmp_disk_pool *pools = NULL; + int ret = 0; + bool bret;
count = virConnectNumOfStoragePools(conn); - if (count <= 0) + if (count < 0) { + ret = count; goto out; + } else if (count == 0) { + goto set_parent; + }
names = calloc(count, sizeof(char *)); if (names == NULL) { CU_DEBUG("Failed to alloc space for %i pool names", count); - count = 0; + ret = -1; goto out; }
- if (virConnectListStoragePools(conn, names, count) == -1) { - CU_DEBUG("Failed to get storage pools"); - count = 0; - goto out; + realcount = virConnectListStoragePools(conn, names, count); + if (realcount < 0) { + CU_DEBUG("Failed to get storage pools, return %d.", realcount); + ret = realcount; + goto free_names; + } + if (realcount == 0) { + CU_DEBUG("Zero pools got, but prelist is %d.", count); + goto set_parent; }
- pools = calloc(count, sizeof(*pools)); + pools = calloc(realcount, sizeof(*pools)); if (pools == NULL) { - CU_DEBUG("Failed to alloc space for %i pool structs", count); - goto out; + CU_DEBUG("Failed to alloc space for %i pool structs", + realcount); + ret = -2; + goto free_names; }
- for (i = 0; i < count; i++) { - pools[i].tag = strdup(names[i]); + for (i = 0; i < realcount; i++) { + pools[i].tag = names[i]; + names[i] = NULL; pools[i].primordial = false; }
- out: - for (i = 0; i < count; i++) - free(names[i]); - free(names); - - get_disk_parent(&pools, &count); + set_parent: + bret = get_disk_parent(&pools, &realcount); + if (bret != true) { + CU_DEBUG("Failed in adding parentpool."); + ret = -4; + goto free_pools;
If here, we will have already called free_diskpools() which is probably a good thing since at this point pools and realcount will be inaccurate. So just jump to free_names instead (also leave a reason why!!!)
+ }
+ /* succeed */ *_pools = pools; + *_count = realcount; + goto free_names; + + free_pools: + free_diskpool(pools, realcount);
- return count; + free_names: + for (i = 0; i < count; i++) { + free(names[i]); + } + free(names); + + out: + return ret; }
static bool diskpool_set_capacity(virConnectPtr conn, @@ -294,42 +349,59 @@ static int parse_diskpool_line(struct tmp_disk_pool *pool, return (ret == 2); }
+/* return 0 on sucess, negative on fail. */ static int get_diskpool_config(virConnectPtr conn, - struct tmp_disk_pool **_pools) + struct tmp_disk_pool **_pools, + int *_count) { const char *path = DISK_POOL_CONFIG; FILE *config; char *line = NULL; size_t len = 0; - int count = 0; - struct tmp_disk_pool *pools = NULL; + int count = 0, ret = 0; + struct tmp_disk_pool *pools = NULL, *new_pools = NULL; + bool bret;
config = fopen(path, "r"); if (config == NULL) { CU_DEBUG("Failed to open %s: %m", path); - return 0; + ret = -1; + goto out; }
while (getline(&line, &len, config) > 0) { - pools = realloc(pools, - (count + 1) * (sizeof(*pools))); - if (pools == NULL) { + new_pools = realloc(pools, + (count + 1) * (sizeof(*pools))); + if (new_pools == NULL) { CU_DEBUG("Failed to alloc new pool"); - goto out; + ret = -2; + goto free_pools; } + pools = new_pools;
Once the realloc is successful, then pools will have count+1 elems. If for some reason the next line files, then I don't think we need to realloc again in this loop. Makes the logic a bit more messy though.
if (parse_diskpool_line(&pools[count], line)) count++;
Since you have a free(line) below, won't you need a free(line) and line = NULL each time through this loop (the line = NULL is so that the free(line) below doesn't double deallocate).
}
+ bret = get_disk_parent(&pools, &count); + if (bret != true) { + CU_DEBUG("Failed in adding parentpool."); + ret = -3; + goto free_pools;
If here, we will have already called free_diskpools() which is probably a good thing since at this point pools and count will be inaccurate. So just jump to clean instead (also leave a reason why!!!)
+ }
- get_disk_parent(&pools, &count); - out: - free(line); + /* succeed */ *_pools = pools; - fclose(config); + *_count = count; + goto clean;
- return count; + free_pools: + free_diskpool(pools, count); + clean: + free(line);
Seeing this free(line) made me realize the issue above... The rest is fine. John
+ fclose(config); + out: + return ret; }
static bool diskpool_set_capacity(virConnectPtr conn, @@ -367,39 +439,23 @@ static bool diskpool_set_capacity(virConnectPtr conn, }
static bool _diskpool_is_member(virConnectPtr conn, - const struct disk_pool *pool, + const struct tmp_disk_pool *pool, const char *file) { return STARTS_WITH(file, pool->path); } #endif
-static void free_diskpool(struct tmp_disk_pool *pools, int count) -{ - int i; - - if (pools == NULL) - return; - - for (i = 0; i < count; i++) { - free(pools[i].tag); - free(pools[i].path); - } - - free(pools); -} - static char *_diskpool_member_of(virConnectPtr conn, const char *file) { struct tmp_disk_pool *pools = NULL; int count; - int i; + int i, ret; char *pool = NULL;
- count = get_diskpool_config(conn, &pools); - if (count == 0) { - free(pools); + ret = get_diskpool_config(conn, &pools, &count); + if (ret < 0) { return NULL; }
@@ -1088,9 +1144,17 @@ static CMPIStatus diskpool_instance(virConnectPtr conn, CMPIStatus s = {CMPI_RC_OK, NULL}; struct tmp_disk_pool *pools = NULL; int count = 0; - int i; + int i, ret;
- count = get_diskpool_config(conn, &pools); + ret = get_diskpool_config(conn, &pools, &count); + if (ret < 0) { + CU_DEBUG("Failed to get diskpool config, return is %d.", ret); + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed to get diskpool config, return is %d.", + ret); + return s; + } if ((id == NULL) && (count == 0)) { CU_DEBUG("No defined DiskPools"); free(pools);

于 2013-3-22 2:03, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Original implemetion may return pools with NULL name if some pool disappear between two libvirt pool API call. And originally it return the number of pools and negative value when error happens, but caller of this function consider number = 0 as error. As a fix, this patch changed the function prototype, it do not return the pool number anymore, it returns 0 on success and negative on fail now. Code for checking the risk of returning pools with NULL name is also added. Another small fix is, return false in get_disk_parent() when strdup fail.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_DevicePool.c | 176 +++++++++++++++++++++++++++++++++---------------- 1 files changed, 120 insertions(+), 56 deletions(-)
diff --git a/src/Virt_DevicePool.c b/src/Virt_DevicePool.c index 08677e2..185e3cc 100644 --- a/src/Virt_DevicePool.c +++ b/src/Virt_DevicePool.c @@ -51,6 +51,22 @@ struct tmp_disk_pool { bool primordial; };
+static void free_diskpool(struct tmp_disk_pool *pools, int count) +{ + int i; + + if (pools == NULL) { + return; + } + + for (i = 0; i < count; i++) { + free(pools[i].tag); + free(pools[i].path); + } + + free(pools); +} + /* * Right now, detect support and use it, if available. * Later, this can be a configure option if needed @@ -78,6 +94,10 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools, }
pools[count].tag = strdup("0"); + if (pools[count].tag == NULL) { + count++; + goto free; + } pools[count].path = NULL; pools[count].primordial = true; count++; @@ -85,12 +105,17 @@ static bool get_disk_parent(struct tmp_disk_pool **_pools, *_count = count; *_pools = pools; ret = true; + goto out;
+ free: + free_diskpool(pools, count); + /* old pool is invalid, update it */ + *_count = 0; + *_pools = NULL; out: return ret; }
- #if VIR_USE_LIBVIRT_STORAGE int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) { @@ -117,52 +142,82 @@ int get_disk_pool(virStoragePoolPtr poolptr, struct virt_pool **pool) return ret; }
+/* This function returns 0 on sucess, negative on fail. */ static int get_diskpool_config(virConnectPtr conn, - struct tmp_disk_pool **_pools) + struct tmp_disk_pool **_pools, + int *_count) { - int count = 0; + int count = 0, realcount = 0; int i; char ** names = NULL; struct tmp_disk_pool *pools = NULL; + int ret = 0; + bool bret;
count = virConnectNumOfStoragePools(conn); - if (count <= 0) + if (count < 0) { + ret = count; goto out; + } else if (count == 0) { + goto set_parent; + }
names = calloc(count, sizeof(char *)); if (names == NULL) { CU_DEBUG("Failed to alloc space for %i pool names", count); - count = 0; + ret = -1; goto out; }
- if (virConnectListStoragePools(conn, names, count) == -1) { - CU_DEBUG("Failed to get storage pools"); - count = 0; - goto out; + realcount = virConnectListStoragePools(conn, names, count); + if (realcount < 0) { + CU_DEBUG("Failed to get storage pools, return %d.", realcount); + ret = realcount; + goto free_names; + } + if (realcount == 0) { + CU_DEBUG("Zero pools got, but prelist is %d.", count); + goto set_parent; }
- pools = calloc(count, sizeof(*pools)); + pools = calloc(realcount, sizeof(*pools)); if (pools == NULL) { - CU_DEBUG("Failed to alloc space for %i pool structs", count); - goto out; + CU_DEBUG("Failed to alloc space for %i pool structs", + realcount); + ret = -2; + goto free_names; }
- for (i = 0; i < count; i++) { - pools[i].tag = strdup(names[i]); + for (i = 0; i < realcount; i++) { + pools[i].tag = names[i]; + names[i] = NULL; pools[i].primordial = false; }
- out: - for (i = 0; i < count; i++) - free(names[i]); - free(names); - - get_disk_parent(&pools, &count); + set_parent: + bret = get_disk_parent(&pools, &realcount); + if (bret != true) { + CU_DEBUG("Failed in adding parentpool."); + ret = -4; + goto free_pools;
If here, we will have already called free_diskpools() which is probably a good thing since at this point pools and realcount will be inaccurate. So just jump to free_names instead (also leave a reason why!!!)
OK, I'll add comment in get_disk_parent() that pools will be freed on fail.
+ }
+ /* succeed */ *_pools = pools; + *_count = realcount; + goto free_names; + + free_pools: + free_diskpool(pools, realcount);
- return count; + free_names: + for (i = 0; i < count; i++) { + free(names[i]); + } + free(names); + + out: + return ret; }
static bool diskpool_set_capacity(virConnectPtr conn, @@ -294,42 +349,59 @@ static int parse_diskpool_line(struct tmp_disk_pool *pool, return (ret == 2); }
+/* return 0 on sucess, negative on fail. */ static int get_diskpool_config(virConnectPtr conn, - struct tmp_disk_pool **_pools) + struct tmp_disk_pool **_pools, + int *_count) { const char *path = DISK_POOL_CONFIG; FILE *config; char *line = NULL; size_t len = 0; - int count = 0; - struct tmp_disk_pool *pools = NULL; + int count = 0, ret = 0; + struct tmp_disk_pool *pools = NULL, *new_pools = NULL; + bool bret;
config = fopen(path, "r"); if (config == NULL) { CU_DEBUG("Failed to open %s: %m", path); - return 0; + ret = -1; + goto out; }
while (getline(&line, &len, config) > 0) { - pools = realloc(pools, - (count + 1) * (sizeof(*pools))); - if (pools == NULL) { + new_pools = realloc(pools, + (count + 1) * (sizeof(*pools))); + if (new_pools == NULL) { CU_DEBUG("Failed to alloc new pool"); - goto out; + ret = -2; + goto free_pools; } + pools = new_pools;
Once the realloc is successful, then pools will have count+1 elems. If for some reason the next line files, then I don't think we need to realloc again in this loop. Makes the logic a bit more messy though.
I'll re-code this old bug like section.
if (parse_diskpool_line(&pools[count], line)) count++;
Since you have a free(line) below, won't you need a free(line) and line = NULL each time through this loop (the line = NULL is so that the free(line) below doesn't double deallocate).
}
+ bret = get_disk_parent(&pools, &count); + if (bret != true) { + CU_DEBUG("Failed in adding parentpool."); + ret = -3; + goto free_pools;
If here, we will have already called free_diskpools() which is probably a good thing since at this point pools and count will be inaccurate. So just jump to clean instead (also leave a reason why!!!)
+ }
- get_disk_parent(&pools, &count); - out: - free(line); + /* succeed */ *_pools = pools; - fclose(config); + *_count = count; + goto clean;
- return count; + free_pools: + free_diskpool(pools, count); + clean: + free(line);
Seeing this free(line) made me realize the issue above...
The rest is fine.
John
+ fclose(config); + out: + return ret; }
static bool diskpool_set_capacity(virConnectPtr conn, @@ -367,39 +439,23 @@ static bool diskpool_set_capacity(virConnectPtr conn, }
static bool _diskpool_is_member(virConnectPtr conn, - const struct disk_pool *pool, + const struct tmp_disk_pool *pool, const char *file) { return STARTS_WITH(file, pool->path); } #endif
-static void free_diskpool(struct tmp_disk_pool *pools, int count) -{ - int i; - - if (pools == NULL) - return; - - for (i = 0; i < count; i++) { - free(pools[i].tag); - free(pools[i].path); - } - - free(pools); -} - static char *_diskpool_member_of(virConnectPtr conn, const char *file) { struct tmp_disk_pool *pools = NULL; int count; - int i; + int i, ret; char *pool = NULL;
- count = get_diskpool_config(conn, &pools); - if (count == 0) { - free(pools); + ret = get_diskpool_config(conn, &pools, &count); + if (ret < 0) { return NULL; }
@@ -1088,9 +1144,17 @@ static CMPIStatus diskpool_instance(virConnectPtr conn, CMPIStatus s = {CMPI_RC_OK, NULL}; struct tmp_disk_pool *pools = NULL; int count = 0; - int i; + int i, ret;
- count = get_diskpool_config(conn, &pools); + ret = get_diskpool_config(conn, &pools, &count); + if (ret < 0) { + CU_DEBUG("Failed to get diskpool config, return is %d.", ret); + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed to get diskpool config, return is %d.", + ret); + return s; + } if ((id == NULL) && (count == 0)) { CU_DEBUG("No defined DiskPools"); free(pools);
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Sharad Mishra <snmishra@us.ibm.com> --- src/Virt_ResourceAllocationSettingDataIndication.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/Virt_ResourceAllocationSettingDataIndication.c b/src/Virt_ResourceAllocationSettingDataIndication.c index a386132..93fb563 100644 --- a/src/Virt_ResourceAllocationSettingDataIndication.c +++ b/src/Virt_ResourceAllocationSettingDataIndication.c @@ -122,7 +122,11 @@ static CMPIStatus raise_indication(const CMPIBroker *broker, if (s.rc == CMPI_RC_OK) { CU_DEBUG("Indication delivered"); } else { - CU_DEBUG("Not delivered: %s", CMGetCharPtr(s.msg)); + if (s.msg == NULL) { + CU_DEBUG("Not delivered: msg is NULL."); + } else { + CU_DEBUG("Not delivered: %s", CMGetCharPtr(s.msg)); + } } out: -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Sharad Mishra <snmishra@us.ibm.com> --- src/Virt_ResourceAllocationSettingDataIndication.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
ACK Again with the comment that you could git rebase -i to include this and 7/15 with 5/15 John

Sometimes libvirt will fail in these APIs if there is low level error, what we saw is libvirt can't got xml for some domain. This patch adds debug log when met this error, so in future we can know what is wrong. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-) diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c index ceb4552..845a953 100644 --- a/libxkutil/device_parsing.c +++ b/libxkutil/device_parsing.c @@ -1251,15 +1251,25 @@ int get_dominfo(virDomainPtr dom, struct domain **dominfo) xml = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); - if (xml == NULL) + if (xml == NULL) { + CU_DEBUG("Failed to get dom xml with libvirt API."); return 0; + } ret = get_dominfo_from_xml(xml, dominfo); - if (virDomainGetAutostart(dom, &start) != 0) - return 0; + if (ret != 1) { + CU_DEBUG("Failed to translate xml into struct domain"); + goto out; + } + if (virDomainGetAutostart(dom, &start) != 0) { + CU_DEBUG("Failed to get dom autostart with libvirt API."); + ret = 0; + goto out; + } (*dominfo)->autostrt = start; + out: free(xml); return ret; -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Sometimes libvirt will fail in these APIs if there is low level error, what we saw is libvirt can't got xml for some domain. This patch adds debug log when met this error, so in future we can know what is wrong.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libxkutil/device_parsing.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-)
ACK John

From cimtest, Calling to virEventRegisterDefaultImpl() of libvirt API, resulting random fail in cases, which seems most likely tog-pegasus's internal data is damaged. The root cause may be: 1 libvirt event API have a bug, we called it from thread A and then do other things in thread B, maybe it did not handle this well. 2 tog-pegasus have confilict with libvirt's event. 3 Potential requirement in libvirt event API or tog-pegasus's thread, which is not document so we used them in a wrong way. Most possible is that tog-pegasus tries to manage all threads resulting the error. This patch bring back libvirt-cim's own old event implemention, which is by default used now. CSI from libvirt can still be activated with a macro. This patch also have changed some buglike code of old libvirt-cim's event implemention. Tested with cimtest on following Env, no more strange error found: RH6.3 libvirt-0.9.10-21.el6.x86_64 tog-pegasus-2.11.0-3.el6.x86_64 Note that to make review easy, this patch try move the code as little as possible, a following "clean up" patch will move the code together. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystem.c | 60 +++- src/Virt_ComputerSystemIndication.c | 663 ++++++++++++++++++++++++++++- src/Virt_VirtualSystemManagementService.c | 57 +++- 3 files changed, 777 insertions(+), 3 deletions(-) diff --git a/src/Virt_ComputerSystem.c b/src/Virt_ComputerSystem.c index 4a9b26d..d37159f 100644 --- a/src/Virt_ComputerSystem.c +++ b/src/Virt_ComputerSystem.c @@ -45,8 +45,58 @@ #include "Virt_HostSystem.h" #include "Virt_VirtualSystemSnapshotService.h" +#include "config.h" + const static CMPIBroker *_BROKER; +#ifndef USE_LIBVIRT_EVENT +static bool trigger_mod_indication(const CMPIBroker *broker, + const CMPIContext *context, + CMPIInstance *prev_inst, + const CMPIObjectPath *ref) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + const char *ind_name = "ComputerSystemModifiedIndication"; + CMPIInstance *ind = NULL; + char *type = NULL; + + CU_DEBUG("Preparing libvirt-cim native ComputerSystem indication"); + + ind = get_typed_instance(broker, + CLASSNAME(ref), + ind_name, + NAMESPACE(ref), + false); + if (ind == NULL) { + CU_DEBUG("Failed to create ind '%s'", ind_name); + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed to create ind '%s'", ind_name); + goto out; + } + + CU_DEBUG("Setting PreviousInstance"); + CMSetProperty(ind, "PreviousInstance", + (CMPIValue *)&prev_inst, CMPI_instance); + + type = get_typed_class(CLASSNAME(ref), ind_name); + + s = stdi_raise_indication(broker, context, type, NAMESPACE(ref), ind); + + out: + free(type); + return s.rc == CMPI_RC_OK; +} +#else +static bool trigger_mod_indication(const CMPIBroker *broker, + const CMPIContext *context, + CMPIInstance *prev_inst, + const CMPIObjectPath *ref) +{ + return true; +} +#endif + /* Set the "Name" property of an instance from a domain */ static int set_name_from_dom(virDomainPtr dom, CMPIInstance *instance) { @@ -1258,8 +1308,16 @@ static CMPIStatus state_change(CMPIMethodMI *self, s = __state_change(name, state, reference); - if (s.rc == CMPI_RC_OK) + if (s.rc == CMPI_RC_OK) { rc = 0; + /* try trigger indication */ + bool ind_rc = trigger_mod_indication(_BROKER, context, + prev_inst, reference); + if (!ind_rc) { + CU_DEBUG("Unable to trigger indication for " + "state change, dom is '%s'", name); + } + } out: CMReturnData(results, &rc, CMPI_uint32); diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index 3df47fa..ef449ff 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -48,7 +48,6 @@ #include "Virt_ComputerSystemIndication.h" #include "Virt_HostSystem.h" - #define CSI_NUM_PLATFORMS 3 enum CSI_PLATFORMS { CSI_XEN, @@ -84,6 +83,8 @@ static pthread_mutex_t lifecycle_mutex = PTHREAD_MUTEX_INITIALIZER; static bool lifecycle_enabled = false; static csi_thread_data_t csi_thread_data[CSI_NUM_PLATFORMS] = {{0}, {0}, {0}}; +#ifndef USE_LIBVIRT_EVENT +#else /* * Domain manipulation */ @@ -180,6 +181,7 @@ static void csi_free_thread_data(void *data) stdi_free_ind_args(&thread->args); pthread_mutex_unlock(&lifecycle_mutex); } +#endif void set_source_inst_props(const CMPIBroker *broker, const CMPIContext *context, @@ -386,6 +388,394 @@ static bool create_deleted_guest_inst(const char *xml, return rc; } +#ifndef USE_LIBVIRT_EVENT +/* libvirt-cim's private CSI implement */ + +#define WAIT_TIME 60 +#define FAIL_WAIT_TIME 2 + +static pthread_cond_t lifecycle_cond = PTHREAD_COND_INITIALIZER; + +struct dom_xml { + char uuid[VIR_UUID_STRING_BUFLEN]; + char *xml; + enum {DOM_OFFLINE, + DOM_ONLINE, + DOM_PAUSED, + DOM_CRASHED, + DOM_GONE, + } state; +}; + +static void free_dom_xml(struct dom_xml dom) +{ + free(dom.xml); + dom.xml = NULL; +} + +static char *sys_name_from_xml(char *xml) +{ + char *tmp = NULL; + char *name = NULL; + int rc; + + tmp = strstr(xml, "<name>"); + if (tmp == NULL) { + goto out; + } + + rc = sscanf(tmp, "<name>%a[^<]s</name>", &name); + if (rc != 1) { + name = NULL; + } + + out: + return name; +} + +static int dom_state(virDomainPtr dom) +{ + virDomainInfo info; + int ret; + + ret = virDomainGetInfo(dom, &info); + if (ret != 0) { + return DOM_GONE; + } + + switch (info.state) { + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_RUNNING: + case VIR_DOMAIN_BLOCKED: + return DOM_ONLINE; + + case VIR_DOMAIN_PAUSED: + return DOM_PAUSED; + + case VIR_DOMAIN_SHUTOFF: + return DOM_OFFLINE; + + case VIR_DOMAIN_CRASHED: + return DOM_CRASHED; + + default: + return DOM_GONE; + }; +} + +static CMPIStatus doms_to_xml(struct dom_xml **dom_xml_list, + virDomainPtr *dom_ptr_list, + int dom_ptr_count) +{ + int i; + int rc; + CMPIStatus s = {CMPI_RC_OK, NULL}; + + if (dom_ptr_count <= 0) { + *dom_xml_list = NULL; + return s; + } + *dom_xml_list = calloc(dom_ptr_count, sizeof(struct dom_xml)); + if (!dom_xml_list) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed calloc %d dom_xml.", dom_ptr_count); + return s; + } + for (i = 0; i < dom_ptr_count; i++) { + rc = virDomainGetUUIDString(dom_ptr_list[i], + (*dom_xml_list)[i].uuid); + if (rc == -1) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get UUID"); + /* If any domain fails, we fail. */ + break; + } + + (*dom_xml_list)[i].xml = virDomainGetXMLDesc(dom_ptr_list[i], + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); + if ((*dom_xml_list)[i].xml == NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to get xml desc"); + break; + } + + (*dom_xml_list)[i].state = dom_state(dom_ptr_list[i]); + } + + return s; +} + +static bool dom_changed(struct dom_xml prev_dom, + struct dom_xml *cur_xml, + int cur_count) +{ + int i; + bool ret = false; + + for (i = 0; i < cur_count; i++) { + if (strcmp(cur_xml[i].uuid, prev_dom.uuid) != 0) { + continue; + } + + if (strcmp(cur_xml[i].xml, prev_dom.xml) != 0) { + CU_DEBUG("Domain config changed"); + ret = true; + } + + if (prev_dom.state != cur_xml[i].state) { + CU_DEBUG("Domain state changed"); + ret = true; + } + + break; + } + + return ret; +} + +static bool wait_for_event(int wait_time) +{ + struct timespec timeout; + int ret; + + + clock_gettime(CLOCK_REALTIME, &timeout); + timeout.tv_sec += wait_time; + + ret = pthread_cond_timedwait(&lifecycle_cond, + &lifecycle_mutex, + &timeout); + return !ret; +} + +static bool dom_in_list(char *uuid, int count, struct dom_xml *list) +{ + int i; + + for (i = 0; i < count; i++) { + if (STREQ(uuid, list[i].uuid)) { + return true; + } + } + + return false; +} + +static bool async_ind_native(CMPIContext *context, + int ind_type, + struct dom_xml prev_dom, + char *prefix, + struct ind_args *args) +{ + bool rc = false; + char *name = NULL; + char *cn = NULL; + CMPIObjectPath *op; + CMPIInstance *prev_inst; + CMPIInstance *affected_inst; + CMPIStatus s = {CMPI_RC_OK, NULL}; + + CU_DEBUG("Entering native indication dilivery with type %d.", ind_type) + if (!lifecycle_enabled) { + CU_DEBUG("CSI not enabled, skipping indication delivery"); + return false; + } + + name = sys_name_from_xml(prev_dom.xml); + CU_DEBUG("Name for system: '%s'", name); + if (name == NULL) { + rc = false; + goto out; + } + + cn = get_typed_class(prefix, "ComputerSystem"); + + op = CMNewObjectPath(_BROKER, args->ns, cn, &s); + if ((s.rc != CMPI_RC_OK) || CMIsNullObject(op)) { + CU_DEBUG("op error"); + goto out; + } + + if (ind_type == CS_CREATED || ind_type == CS_MODIFIED) { + s = get_domain_by_name(_BROKER, op, name, &affected_inst); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("domain by name error"); + goto out; + } + } else if (ind_type == CS_DELETED) { + rc = create_deleted_guest_inst(prev_dom.xml, + args->ns, + prefix, + &affected_inst); + if (!rc) { + CU_DEBUG("Could not recreate guest instance"); + goto out; + } + } else { + CU_DEBUG("Unrecognized indication type %d", ind_type); + goto out; + } + + /* FIXME: We are unable to get the previous CS instance after it has + been modified. Consider keeping track of the previous + state in the place we keep track of the requested state */ + prev_inst = affected_inst; + + CMSetProperty(affected_inst, "Name", + (CMPIValue *)name, CMPI_chars); + CMSetProperty(affected_inst, "UUID", + (CMPIValue *)prev_dom.uuid, CMPI_chars); + + rc = _do_indication(_BROKER, context, prev_inst, affected_inst, + ind_type, prefix, args); + + out: + free(cn); + free(name); + return rc; +} + +static CMPI_THREAD_RETURN lifecycle_thread_native(void *params) +{ + CU_DEBUG("Entering libvirtc-cim native CSI thread."); + csi_thread_data_t *thread = (csi_thread_data_t *) params; + struct ind_args *args = thread->args; + CMPIContext *context = args->context; + char *prefix = class_prefix_name(args->classname); + virConnectPtr conn; + CMPIStatus s; + int retry_time = FAIL_WAIT_TIME; + + struct dom_xml *cur_xml = NULL; + struct dom_xml *prev_xml = NULL; + int prev_count = 0; + int cur_count = 0; + virDomainPtr *tmp_list = NULL; + int CBAttached = 0; + + if (prefix == NULL) { + goto init_out; + } + + pthread_mutex_lock(&lifecycle_mutex); + conn = connect_by_classname(_BROKER, args->classname, &s); + if (conn == NULL) { + CU_DEBUG("Unable to start lifecycle thread: " + "Failed to connect (cn: %s)", args->classname); + pthread_mutex_unlock(&lifecycle_mutex); + goto conn_out; + } + + CBAttachThread(_BROKER, args->context); + CBAttached = 1; + prev_count = get_domain_list(conn, &tmp_list); + s = doms_to_xml(&prev_xml, tmp_list, prev_count); + free_domain_list(tmp_list, prev_count); + free(tmp_list); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("doms_to_xml failed. Attempting to continue."); + } + + CU_DEBUG("Entering libvirt-cim native CSI event loop (%s)", prefix); + + int i; + while (1) { + if (thread->active_filters <= 0) { + break; + } + + bool res; + bool failure = false; + + cur_count = get_domain_list(conn, &tmp_list); + s = doms_to_xml(&cur_xml, tmp_list, cur_count); + free_domain_list(tmp_list, cur_count); + free(tmp_list); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("doms_to_xml failed. retry in %d seconds", + retry_time); + failure = true; + goto fail; + } + + /* CU_DEBUG("cur_count %d, prev_count %d.", + cur_count, prev_count); */ + for (i = 0; i < cur_count; i++) { + res = dom_in_list(cur_xml[i].uuid, + prev_count, prev_xml); + if (!res) { + async_ind_native(context, CS_CREATED, + cur_xml[i], prefix, args); + } + + } + + for (i = 0; i < prev_count; i++) { + res = dom_in_list(prev_xml[i].uuid, + cur_count, cur_xml); + if (!res) { + async_ind_native(context, CS_DELETED, + prev_xml[i], prefix, args); + } else if (dom_changed(prev_xml[i], + cur_xml, cur_count)) { + async_ind_native(context, CS_MODIFIED, + prev_xml[i], prefix, args); + } + free_dom_xml(prev_xml[i]); + } + + fail: + if (failure) { + wait_for_event(FAIL_WAIT_TIME); + } else { + free(prev_xml); + prev_xml = cur_xml; + cur_xml = NULL; + prev_count = cur_count; + cur_count = 0; + wait_for_event(WAIT_TIME); + } + } + + CU_DEBUG("Exiting libvirt-cim native CSI event loop (%s)", prefix); + + if (prev_xml != NULL) { + for (i = 0; i < prev_count; i++) { + free_dom_xml(prev_xml[i]); + } + free(prev_xml); + prev_xml = NULL; + } + + pthread_mutex_unlock(&lifecycle_mutex); + + virConnectClose(conn); + + conn_out: + free(prefix); + + init_out: + pthread_mutex_lock(&lifecycle_mutex); + thread->id = 0; + thread->active_filters = 0; + + /* it seems tog-pegasus try kill this thread after detached, use this + flag to delay detach as much as possible. */ + if (CBAttached > 0) { + CBDetachThread(_BROKER, args->context); + } + if (thread->args != NULL) { + stdi_free_ind_args(&thread->args); + } + + pthread_mutex_unlock(&lifecycle_mutex); + + return (CMPI_THREAD_RETURN) 0; +} +#else static bool async_ind(struct ind_args *args, int ind_type, csi_dom_xml_t *dom, @@ -669,6 +1059,7 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) free(prefix); return (CMPI_THREAD_RETURN) 0; } +#endif static int platform_from_class(const char *cn) { @@ -682,6 +1073,162 @@ static int platform_from_class(const char *cn) return -1; } +#ifndef USE_LIBVIRT_EVENT +static CMPIStatus ActivateFilter(CMPIIndicationMI *mi, + const CMPIContext *ctx, + const CMPISelectExp *se, + const char *ns, + const CMPIObjectPath *op, + CMPIBoolean first) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + struct std_indication_ctx *_ctx; + struct ind_args *args = NULL; + int platform; + bool error = false; + csi_thread_data_t *thread = NULL; + + CU_DEBUG("ActivateFilter for %s", CLASSNAME(op)); + + pthread_mutex_lock(&lifecycle_mutex); + + CU_DEBUG("Using libvirt-cim's event implemention."); + + _ctx = (struct std_indication_ctx *)mi->hdl; + + if (CMIsNullObject(op)) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "No ObjectPath given"); + goto out; + } + + /* FIXME: op is stale the second time around, for some reason */ + platform = platform_from_class(CLASSNAME(op)); + if (platform < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unknown platform"); + goto out; + } + + thread = &csi_thread_data[platform]; + thread->active_filters += 1; + + /* Check if thread is already running */ + if (thread->id > 0) { + goto out; + } + + args = malloc(sizeof(*args)); + if (args == NULL) { + CU_DEBUG("Failed to allocate ind_args"); + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Unable to allocate ind_args"); + error = true; + goto out; + } + + args->context = CBPrepareAttachThread(_BROKER, ctx); + if (args->context == NULL) { + CU_DEBUG("Failed to create thread context"); + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Unable to create thread context"); + error = true; + goto out; + } + + args->ns = strdup(NAMESPACE(op)); + args->classname = strdup(CLASSNAME(op)); + args->_ctx = _ctx; + + thread->args = args; + + thread->id = _BROKER->xft->newThread(lifecycle_thread_native, + thread, 0); + + if (thread->id <= 0) { + CU_DEBUG("Error, failed to create new thread."); + error = true; + } + + out: + if (error == true) { + thread->active_filters -= 1; + free(args); + } + + pthread_mutex_unlock(&lifecycle_mutex); + + return s; +} + +static CMPIStatus DeActivateFilter(CMPIIndicationMI *mi, + const CMPIContext *ctx, + const CMPISelectExp *se, + const char *ns, + const CMPIObjectPath *op, + CMPIBoolean last) +{ + int platform; + CMPIStatus s = {CMPI_RC_OK, NULL}; + + CU_DEBUG("DeActivateFilter for %s", CLASSNAME(op)); + + platform = platform_from_class(CLASSNAME(op)); + if (platform < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unknown platform"); + goto out; + } + + + pthread_mutex_lock(&lifecycle_mutex); + csi_thread_data[platform].active_filters -= 1; + pthread_mutex_unlock(&lifecycle_mutex); + + pthread_cond_signal(&lifecycle_cond); + + out: + return s; +} + +static CMPIStatus trigger_indication(const CMPIContext *context) +{ + CU_DEBUG("triggered"); + pthread_cond_signal(&lifecycle_cond); + return (CMPIStatus){CMPI_RC_OK, NULL}; +} + +static CMPIInstance *get_prev_inst(const CMPIBroker *broker, + const CMPIInstance *ind, + CMPIStatus *s) +{ + CMPIData data; + CMPIInstance *prev_inst = NULL; + + data = CMGetProperty(ind, "PreviousInstance", s); + if (s->rc != CMPI_RC_OK || CMIsNullValue(data)) { + cu_statusf(broker, s, + CMPI_RC_ERR_NO_SUCH_PROPERTY, + "Unable to get PreviousInstance of the indication"); + goto out; + } + + if (data.type != CMPI_instance) { + cu_statusf(broker, s, + CMPI_RC_ERR_TYPE_MISMATCH, + "Indication SourceInstance is of unexpected type"); + goto out; + } + + prev_inst = data.value.inst; + + out: + return prev_inst; +} +#else static CMPIStatus ActivateFilter(CMPIIndicationMI* mi, const CMPIContext* ctx, const CMPISelectExp* se, @@ -703,6 +1250,7 @@ static CMPIStatus ActivateFilter(CMPIIndicationMI* mi, if (events_registered == 0) { events_registered = 1; + CU_DEBUG("Registering libvirt event."); virEventRegisterDefaultImpl(); } @@ -756,6 +1304,11 @@ static CMPIStatus ActivateFilter(CMPIIndicationMI* mi, thread->args = args; thread->id = _BROKER->xft->newThread(lifecycle_thread, thread, 0); + if (thread->id <= 0) { + CU_DEBUG("Error, failed to create new thread."); + error = true; + } + out: if (error == true) { thread->active_filters -= 1; @@ -795,6 +1348,7 @@ static CMPIStatus DeActivateFilter(CMPIIndicationMI* mi, out: return s; } +#endif static _EI_RTYPE EnableIndications(CMPIIndicationMI* mi, const CMPIContext *ctx) @@ -841,7 +1395,114 @@ static struct std_ind_filter *filters[] = { NULL, }; +#ifndef USE_LIBVIRT_EVENT +static CMPIStatus raise_indication(const CMPIBroker *broker, + const CMPIContext *ctx, + const CMPIObjectPath *ref, + const CMPIInstance *ind) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *prev_inst; + CMPIInstance *src_inst; + CMPIObjectPath *_ref = NULL; + struct std_indication_ctx *_ctx = NULL; + struct ind_args *args = NULL; + char *prefix = NULL; + bool rc; + + if (!lifecycle_enabled) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "CSI not enabled, skipping indication delivery"); + goto out; + } + + prev_inst = get_prev_inst(broker, ind, &s); + if (s.rc != CMPI_RC_OK || CMIsNullObject(prev_inst)) { + goto out; + } + + _ref = CMGetObjectPath(prev_inst, &s); + if (s.rc != CMPI_RC_OK) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to get a reference to the guest"); + goto out; + } + + /* FIXME: This is a Pegasus work around. Pegsus loses the namespace + when an ObjectPath is pulled from an instance */ + if (STREQ(NAMESPACE(_ref), "")) { + CMSetNameSpace(_ref, "root/virt"); + } + + s = get_domain_by_ref(broker, _ref, &src_inst); + if (s.rc != CMPI_RC_OK || CMIsNullObject(src_inst)) { + goto out; + } + + _ctx = malloc(sizeof(struct std_indication_ctx)); + if (_ctx == NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to allocate indication context"); + goto out; + } + + _ctx->brkr = broker; + _ctx->handler = NULL; + _ctx->filters = filters; + _ctx->enabled = lifecycle_enabled; + + args = malloc(sizeof(struct ind_args)); + if (args == NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to allocate ind_args"); + goto out; + } + + args->ns = strdup(NAMESPACE(_ref)); + args->classname = strdup(CLASSNAME(_ref)); + if (!args->classname || !args->ns) { + CU_DEBUG("Failed in strdup"); + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed in strdup in indication raising"); + goto out; + } + args->_ctx = _ctx; + + prefix = class_prefix_name(args->classname); + + rc = _do_indication(broker, ctx, prev_inst, src_inst, + CS_MODIFIED, prefix, args); + + if (!rc) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to generate indication"); + } + + out: + if (args != NULL) { + stdi_free_ind_args(&args); + } + + if (_ctx != NULL) { + free(_ctx); + } + + free(prefix); + return s; +} +#endif + static struct std_indication_handler csi = { +#ifndef USE_LIBVIRT_EVENT + .raise_fn = raise_indication, + .trigger_fn = trigger_indication, +#endif .activate_fn = ActivateFilter, .deactivate_fn = DeActivateFilter, .enable_fn = EnableIndications, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c index 96c8a03..4da92e3 100644 --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -107,6 +107,33 @@ enum ResourceAction { RESOURCE_MOD, }; +#ifndef USE_LIBVIRT_EVENT +static bool trigger_indication(const CMPIBroker *broker, + const CMPIContext *context, + const char *base_type, + const CMPIObjectPath *ref) +{ + char *type; + CMPIStatus s; + + type = get_typed_class(CLASSNAME(ref), base_type); + + s = stdi_trigger_indication(broker, context, type, NAMESPACE(ref)); + + free(type); + + return s.rc == CMPI_RC_OK; +} +#else +static bool trigger_indication(const CMPIBroker *broker; + const CMPIContext *context, + const char *base_type, + const CMPIObjectPath *ref) +{ + return true; +} +#endif + #if LIBVIR_VERSION_NUMBER < 9000 /* Network QoS support */ static CMPIStatus add_qos_for_mac(const uint64_t qos, @@ -2167,6 +2194,16 @@ static CMPIStatus define_system(CMPIMethodMI *self, CMAddArg(argsout, "ResultingSystem", &result, CMPI_ref); } + /* try trigger indication */ + bool ind_rc = trigger_indication(_BROKER, context, + "ComputerSystemCreatedIndication", reference); + if (!ind_rc) { + const char *dom_name = NULL; + cu_get_str_prop(vssd, "VirtualSystemIdentifier", &dom_name); + CU_DEBUG("Unable to trigger indication for " + "system create, dom is '%s'", dom_name); + } + out: if (s.rc == CMPI_RC_OK) rc = CIM_SVPC_RETURN_COMPLETED; @@ -2269,6 +2306,15 @@ error: NULL, reference, &list); + + /* try trigger indication */ + bool ind_rc = trigger_indication(_BROKER, context, + "ComputerSystemDeletedIndication", reference); + if (!ind_rc) { + CU_DEBUG("Unable to trigger indication for " + "system delete, dom is '%s'", dom_name); + } + } virDomainFree(dom); @@ -2350,8 +2396,17 @@ static CMPIStatus update_system_settings(const CMPIContext *context, connect_and_create(xml, ref, &s); } - if (s.rc == CMPI_RC_OK) + if (s.rc == CMPI_RC_OK) { set_autostart(vssd, ref, dom); + /* try trigger indication */ + bool ind_rc = trigger_indication(_BROKER, context, + "ComputerSystemModifiedIndication", ref); + if (!ind_rc) { + CU_DEBUG("Unable to trigger indication for " + "system modify, dom is '%s'", name); + } + + } out: free(xml); -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
From cimtest, Calling to virEventRegisterDefaultImpl() of libvirt API, resulting random fail in cases, which seems most likely tog-pegasus's internal data is damaged. The root cause may be: 1 libvirt event API have a bug, we called it from thread A and then do other things in thread B, maybe it did not handle this well. 2 tog-pegasus have confilict with libvirt's event. 3 Potential requirement in libvirt event API or tog-pegasus's thread, which is not document so we used them in a wrong way.
Most possible is that tog-pegasus tries to manage all threads resulting the error.
This patch bring back libvirt-cim's own old event implemention, which is by default used now. CSI from libvirt can still be activated with a macro. This patch also have changed some buglike code of old libvirt-cim's event implemention. Tested with cimtest on following Env, no more strange error found: RH6.3 libvirt-0.9.10-21.el6.x86_64 tog-pegasus-2.11.0-3.el6.x86_64
Note that to make review easy, this patch try move the code as little as possible, a following "clean up" patch will move the code together.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystem.c | 60 +++- src/Virt_ComputerSystemIndication.c | 663 ++++++++++++++++++++++++++++- src/Virt_VirtualSystemManagementService.c | 57 +++- 3 files changed, 777 insertions(+), 3 deletions(-)
ACK John

This is a pure code move patch. Now codes using libvirt event or native event, are moved into one macro protection, which make code easy to read, and in futher they can be moved into new file as CSI-libvirt.c. This patch also fix code style problem in moved code. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 851 ++++++++++++++++++----------------- 1 files changed, 429 insertions(+), 422 deletions(-) diff --git a/src/Virt_ComputerSystemIndication.c b/src/Virt_ComputerSystemIndication.c index ef449ff..247143c 100644 --- a/src/Virt_ComputerSystemIndication.c +++ b/src/Virt_ComputerSystemIndication.c @@ -83,106 +83,6 @@ static pthread_mutex_t lifecycle_mutex = PTHREAD_MUTEX_INITIALIZER; static bool lifecycle_enabled = false; static csi_thread_data_t csi_thread_data[CSI_NUM_PLATFORMS] = {{0}, {0}, {0}}; -#ifndef USE_LIBVIRT_EVENT -#else -/* - * Domain manipulation - */ -static void csi_dom_xml_free(void *data) -{ - csi_dom_xml_t *dom = (csi_dom_xml_t *) data; - free(dom->xml); - free(dom->name); - free(dom); -} - -static int csi_dom_xml_cmp(void *data, void *cmp_cb_data) -{ - csi_dom_xml_t *dom = (csi_dom_xml_t *) data; - const char *uuid = (const char *) cmp_cb_data; - - return strcmp(dom->uuid, uuid); -} - -static int csi_dom_xml_set(csi_dom_xml_t *dom, virDomainPtr dom_ptr, CMPIStatus *s) -{ - const char *name; - - name = virDomainGetName(dom_ptr); - if (name == NULL) { - cu_statusf(_BROKER, s, - CMPI_RC_ERR_FAILED, - "Failed to get domain name"); - return -1; - } - - dom->name = strdup(name); - - /* xml */ - dom->xml = virDomainGetXMLDesc(dom_ptr, - VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); - if (dom->xml == NULL) { - cu_statusf(_BROKER, s, - CMPI_RC_ERR_FAILED, - "Failed to get xml desc"); - return -1; - } - - return 0; -} - -static csi_dom_xml_t *csi_dom_xml_new(virDomainPtr dom_ptr, CMPIStatus *s) -{ - int rc; - csi_dom_xml_t *dom; - - dom = calloc(1, sizeof(*dom)); - if (dom == NULL) - return NULL; - - /* uuid */ - rc = virDomainGetUUIDString(dom_ptr, dom->uuid); - if (rc == -1) { - cu_statusf(_BROKER, s, - CMPI_RC_ERR_FAILED, - "Failed to get domain UUID"); - goto error; - } - - if (csi_dom_xml_set(dom, dom_ptr, s) == -1) - goto error; - - return dom; - - error: - csi_dom_xml_free(dom); - return NULL; -} - -static void csi_thread_dom_list_append(csi_thread_data_t *thread, - csi_dom_xml_t *dom) -{ - if (thread->dom_list == NULL) - thread->dom_list = list_new(csi_dom_xml_free, csi_dom_xml_cmp); - - list_append(thread->dom_list, dom); -} - -static void csi_free_thread_data(void *data) -{ - csi_thread_data_t *thread = (csi_thread_data_t *) data; - - if (data == NULL) - return; - - pthread_mutex_lock(&lifecycle_mutex); - list_free(thread->dom_list); - thread->dom_list = NULL; - stdi_free_ind_args(&thread->args); - pthread_mutex_unlock(&lifecycle_mutex); -} -#endif - void set_source_inst_props(const CMPIBroker *broker, const CMPIContext *context, const CMPIObjectPath *ref, @@ -388,6 +288,64 @@ static bool create_deleted_guest_inst(const char *xml, return rc; } +static int platform_from_class(const char *cn) +{ + if (STARTS_WITH(cn, "Xen")) { + return CSI_XEN; + } else if (STARTS_WITH(cn, "KVM")) { + return CSI_KVM; + } else if (STARTS_WITH(cn, "LXC")) { + return CSI_LXC; + } else { + return -1; + } +} + +static _EI_RTYPE EnableIndications(CMPIIndicationMI *mi, + const CMPIContext *ctx) +{ + CU_DEBUG("EnableIndications"); + pthread_mutex_lock(&lifecycle_mutex); + lifecycle_enabled = true; + pthread_mutex_unlock(&lifecycle_mutex); + + _EI_RET(); +} + +static _EI_RTYPE DisableIndications(CMPIIndicationMI *mi, + const CMPIContext *ctx) +{ + CU_DEBUG("DisableIndications"); + pthread_mutex_lock(&lifecycle_mutex); + lifecycle_enabled = false; + pthread_mutex_unlock(&lifecycle_mutex); + + _EI_RET(); +} + +DECLARE_FILTER(xen_created, "Xen_ComputerSystemCreatedIndication"); +DECLARE_FILTER(xen_deleted, "Xen_ComputerSystemDeletedIndication"); +DECLARE_FILTER(xen_modified, "Xen_ComputerSystemModifiedIndication"); +DECLARE_FILTER(kvm_created, "KVM_ComputerSystemCreatedIndication"); +DECLARE_FILTER(kvm_deleted, "KVM_ComputerSystemDeletedIndication"); +DECLARE_FILTER(kvm_modified, "KVM_ComputerSystemModifiedIndication"); +DECLARE_FILTER(lxc_created, "LXC_ComputerSystemCreatedIndication"); +DECLARE_FILTER(lxc_deleted, "LXC_ComputerSystemDeletedIndication"); +DECLARE_FILTER(lxc_modified, "LXC_ComputerSystemModifiedIndication"); + +static struct std_ind_filter *filters[] = { + &xen_created, + &xen_deleted, + &xen_modified, + &kvm_created, + &kvm_deleted, + &kvm_modified, + &lxc_created, + &lxc_deleted, + &lxc_modified, + NULL, +}; + #ifndef USE_LIBVIRT_EVENT /* libvirt-cim's private CSI implement */ @@ -775,7 +733,377 @@ static CMPI_THREAD_RETURN lifecycle_thread_native(void *params) return (CMPI_THREAD_RETURN) 0; } + +static CMPIStatus ActivateFilter(CMPIIndicationMI *mi, + const CMPIContext *ctx, + const CMPISelectExp *se, + const char *ns, + const CMPIObjectPath *op, + CMPIBoolean first) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + struct std_indication_ctx *_ctx; + struct ind_args *args = NULL; + int platform; + bool error = false; + csi_thread_data_t *thread = NULL; + + CU_DEBUG("ActivateFilter for %s", CLASSNAME(op)); + + pthread_mutex_lock(&lifecycle_mutex); + + CU_DEBUG("Using libvirt-cim's event implemention."); + + _ctx = (struct std_indication_ctx *)mi->hdl; + + if (CMIsNullObject(op)) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "No ObjectPath given"); + goto out; + } + + /* FIXME: op is stale the second time around, for some reason */ + platform = platform_from_class(CLASSNAME(op)); + if (platform < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unknown platform"); + goto out; + } + + thread = &csi_thread_data[platform]; + thread->active_filters += 1; + + /* Check if thread is already running */ + if (thread->id > 0) { + goto out; + } + + args = malloc(sizeof(*args)); + if (args == NULL) { + CU_DEBUG("Failed to allocate ind_args"); + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Unable to allocate ind_args"); + error = true; + goto out; + } + + args->context = CBPrepareAttachThread(_BROKER, ctx); + if (args->context == NULL) { + CU_DEBUG("Failed to create thread context"); + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Unable to create thread context"); + error = true; + goto out; + } + + args->ns = strdup(NAMESPACE(op)); + args->classname = strdup(CLASSNAME(op)); + args->_ctx = _ctx; + + thread->args = args; + + thread->id = _BROKER->xft->newThread(lifecycle_thread_native, + thread, 0); + + if (thread->id <= 0) { + CU_DEBUG("Error, failed to create new thread."); + error = true; + } + + out: + if (error == true) { + thread->active_filters -= 1; + free(args); + } + + pthread_mutex_unlock(&lifecycle_mutex); + + return s; +} + +static CMPIStatus DeActivateFilter(CMPIIndicationMI *mi, + const CMPIContext *ctx, + const CMPISelectExp *se, + const char *ns, + const CMPIObjectPath *op, + CMPIBoolean last) +{ + int platform; + CMPIStatus s = {CMPI_RC_OK, NULL}; + + CU_DEBUG("DeActivateFilter for %s", CLASSNAME(op)); + + platform = platform_from_class(CLASSNAME(op)); + if (platform < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unknown platform"); + goto out; + } + + + pthread_mutex_lock(&lifecycle_mutex); + csi_thread_data[platform].active_filters -= 1; + pthread_mutex_unlock(&lifecycle_mutex); + + pthread_cond_signal(&lifecycle_cond); + + out: + return s; +} + +static CMPIStatus trigger_indication(const CMPIContext *context) +{ + CU_DEBUG("triggered"); + pthread_cond_signal(&lifecycle_cond); + return (CMPIStatus){CMPI_RC_OK, NULL}; +} + +static CMPIInstance *get_prev_inst(const CMPIBroker *broker, + const CMPIInstance *ind, + CMPIStatus *s) +{ + CMPIData data; + CMPIInstance *prev_inst = NULL; + + data = CMGetProperty(ind, "PreviousInstance", s); + if (s->rc != CMPI_RC_OK || CMIsNullValue(data)) { + cu_statusf(broker, s, + CMPI_RC_ERR_NO_SUCH_PROPERTY, + "Unable to get PreviousInstance of the indication"); + goto out; + } + + if (data.type != CMPI_instance) { + cu_statusf(broker, s, + CMPI_RC_ERR_TYPE_MISMATCH, + "Indication SourceInstance is of unexpected type"); + goto out; + } + + prev_inst = data.value.inst; + + out: + return prev_inst; +} + +static CMPIStatus raise_indication(const CMPIBroker *broker, + const CMPIContext *ctx, + const CMPIObjectPath *ref, + const CMPIInstance *ind) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *prev_inst; + CMPIInstance *src_inst; + CMPIObjectPath *_ref = NULL; + struct std_indication_ctx *_ctx = NULL; + struct ind_args *args = NULL; + char *prefix = NULL; + bool rc; + + if (!lifecycle_enabled) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "CSI not enabled, skipping indication delivery"); + goto out; + } + + prev_inst = get_prev_inst(broker, ind, &s); + if (s.rc != CMPI_RC_OK || CMIsNullObject(prev_inst)) { + goto out; + } + + _ref = CMGetObjectPath(prev_inst, &s); + if (s.rc != CMPI_RC_OK) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to get a reference to the guest"); + goto out; + } + + /* FIXME: This is a Pegasus work around. Pegsus loses the namespace + when an ObjectPath is pulled from an instance */ + if (STREQ(NAMESPACE(_ref), "")) { + CMSetNameSpace(_ref, "root/virt"); + } + + s = get_domain_by_ref(broker, _ref, &src_inst); + if (s.rc != CMPI_RC_OK || CMIsNullObject(src_inst)) { + goto out; + } + + _ctx = malloc(sizeof(struct std_indication_ctx)); + if (_ctx == NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to allocate indication context"); + goto out; + } + + _ctx->brkr = broker; + _ctx->handler = NULL; + _ctx->filters = filters; + _ctx->enabled = lifecycle_enabled; + + args = malloc(sizeof(struct ind_args)); + if (args == NULL) { + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Unable to allocate ind_args"); + goto out; + } + + args->ns = strdup(NAMESPACE(_ref)); + args->classname = strdup(CLASSNAME(_ref)); + if (!args->classname || !args->ns) { + CU_DEBUG("Failed in strdup"); + cu_statusf(broker, &s, + CMPI_RC_ERR_FAILED, + "Failed in strdup in indication raising"); + goto out; + } + args->_ctx = _ctx; + + prefix = class_prefix_name(args->classname); + + rc = _do_indication(broker, ctx, prev_inst, src_inst, + CS_MODIFIED, prefix, args); + + if (!rc) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to generate indication"); + } + + out: + if (args != NULL) { + stdi_free_ind_args(&args); + } + + if (_ctx != NULL) { + free(_ctx); + } + + free(prefix); + return s; +} + +static struct std_indication_handler csi = { + .raise_fn = raise_indication, + .trigger_fn = trigger_indication, + .activate_fn = ActivateFilter, + .deactivate_fn = DeActivateFilter, + .enable_fn = EnableIndications, + .disable_fn = DisableIndications, +}; #else +/* Using libvirt's event to implement CSI */ + +/* + * Domain manipulation + */ +static void csi_dom_xml_free(void *data) +{ + csi_dom_xml_t *dom = (csi_dom_xml_t *) data; + free(dom->xml); + free(dom->name); + free(dom); +} + +static int csi_dom_xml_cmp(void *data, void *cmp_cb_data) +{ + csi_dom_xml_t *dom = (csi_dom_xml_t *) data; + const char *uuid = (const char *) cmp_cb_data; + + return strcmp(dom->uuid, uuid); +} + +static int csi_dom_xml_set(csi_dom_xml_t *dom, + virDomainPtr dom_ptr, + CMPIStatus *s) +{ + const char *name; + + name = virDomainGetName(dom_ptr); + if (name == NULL) { + cu_statusf(_BROKER, s, + CMPI_RC_ERR_FAILED, + "Failed to get domain name"); + return -1; + } + + dom->name = strdup(name); + + /* xml */ + dom->xml = virDomainGetXMLDesc(dom_ptr, + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE); + if (dom->xml == NULL) { + cu_statusf(_BROKER, s, + CMPI_RC_ERR_FAILED, + "Failed to get xml desc"); + return -1; + } + + return 0; +} + +static csi_dom_xml_t *csi_dom_xml_new(virDomainPtr dom_ptr, CMPIStatus *s) +{ + int rc; + csi_dom_xml_t *dom; + + dom = calloc(1, sizeof(*dom)); + if (dom == NULL) { + return NULL; + } + + /* uuid */ + rc = virDomainGetUUIDString(dom_ptr, dom->uuid); + if (rc == -1) { + cu_statusf(_BROKER, s, + CMPI_RC_ERR_FAILED, + "Failed to get domain UUID"); + goto error; + } + + if (csi_dom_xml_set(dom, dom_ptr, s) == -1) { + goto error; + } + + return dom; + + error: + csi_dom_xml_free(dom); + return NULL; +} + +static void csi_thread_dom_list_append(csi_thread_data_t *thread, + csi_dom_xml_t *dom) +{ + if (thread->dom_list == NULL) { + thread->dom_list = list_new(csi_dom_xml_free, csi_dom_xml_cmp); + } + + list_append(thread->dom_list, dom); +} + +static void csi_free_thread_data(void *data) +{ + csi_thread_data_t *thread = (csi_thread_data_t *) data; + + if (data == NULL) { + return; + } + + pthread_mutex_lock(&lifecycle_mutex); + list_free(thread->dom_list); + thread->dom_list = NULL; + stdi_free_ind_args(&thread->args); + pthread_mutex_unlock(&lifecycle_mutex); +} + static bool async_ind(struct ind_args *args, int ind_type, csi_dom_xml_t *dom, @@ -1059,176 +1387,7 @@ static CMPI_THREAD_RETURN lifecycle_thread(void *params) free(prefix); return (CMPI_THREAD_RETURN) 0; } -#endif - -static int platform_from_class(const char *cn) -{ - if (STARTS_WITH(cn, "Xen")) - return CSI_XEN; - else if (STARTS_WITH(cn, "KVM")) - return CSI_KVM; - else if (STARTS_WITH(cn, "LXC")) - return CSI_LXC; - else - return -1; -} - -#ifndef USE_LIBVIRT_EVENT -static CMPIStatus ActivateFilter(CMPIIndicationMI *mi, - const CMPIContext *ctx, - const CMPISelectExp *se, - const char *ns, - const CMPIObjectPath *op, - CMPIBoolean first) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - struct std_indication_ctx *_ctx; - struct ind_args *args = NULL; - int platform; - bool error = false; - csi_thread_data_t *thread = NULL; - - CU_DEBUG("ActivateFilter for %s", CLASSNAME(op)); - - pthread_mutex_lock(&lifecycle_mutex); - - CU_DEBUG("Using libvirt-cim's event implemention."); - - _ctx = (struct std_indication_ctx *)mi->hdl; - - if (CMIsNullObject(op)) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "No ObjectPath given"); - goto out; - } - - /* FIXME: op is stale the second time around, for some reason */ - platform = platform_from_class(CLASSNAME(op)); - if (platform < 0) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Unknown platform"); - goto out; - } - - thread = &csi_thread_data[platform]; - thread->active_filters += 1; - - /* Check if thread is already running */ - if (thread->id > 0) { - goto out; - } - - args = malloc(sizeof(*args)); - if (args == NULL) { - CU_DEBUG("Failed to allocate ind_args"); - cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, - "Unable to allocate ind_args"); - error = true; - goto out; - } - - args->context = CBPrepareAttachThread(_BROKER, ctx); - if (args->context == NULL) { - CU_DEBUG("Failed to create thread context"); - cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, - "Unable to create thread context"); - error = true; - goto out; - } - - args->ns = strdup(NAMESPACE(op)); - args->classname = strdup(CLASSNAME(op)); - args->_ctx = _ctx; - - thread->args = args; - - thread->id = _BROKER->xft->newThread(lifecycle_thread_native, - thread, 0); - - if (thread->id <= 0) { - CU_DEBUG("Error, failed to create new thread."); - error = true; - } - - out: - if (error == true) { - thread->active_filters -= 1; - free(args); - } - - pthread_mutex_unlock(&lifecycle_mutex); - - return s; -} - -static CMPIStatus DeActivateFilter(CMPIIndicationMI *mi, - const CMPIContext *ctx, - const CMPISelectExp *se, - const char *ns, - const CMPIObjectPath *op, - CMPIBoolean last) -{ - int platform; - CMPIStatus s = {CMPI_RC_OK, NULL}; - - CU_DEBUG("DeActivateFilter for %s", CLASSNAME(op)); - platform = platform_from_class(CLASSNAME(op)); - if (platform < 0) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Unknown platform"); - goto out; - } - - - pthread_mutex_lock(&lifecycle_mutex); - csi_thread_data[platform].active_filters -= 1; - pthread_mutex_unlock(&lifecycle_mutex); - - pthread_cond_signal(&lifecycle_cond); - - out: - return s; -} - -static CMPIStatus trigger_indication(const CMPIContext *context) -{ - CU_DEBUG("triggered"); - pthread_cond_signal(&lifecycle_cond); - return (CMPIStatus){CMPI_RC_OK, NULL}; -} - -static CMPIInstance *get_prev_inst(const CMPIBroker *broker, - const CMPIInstance *ind, - CMPIStatus *s) -{ - CMPIData data; - CMPIInstance *prev_inst = NULL; - - data = CMGetProperty(ind, "PreviousInstance", s); - if (s->rc != CMPI_RC_OK || CMIsNullValue(data)) { - cu_statusf(broker, s, - CMPI_RC_ERR_NO_SUCH_PROPERTY, - "Unable to get PreviousInstance of the indication"); - goto out; - } - - if (data.type != CMPI_instance) { - cu_statusf(broker, s, - CMPI_RC_ERR_TYPE_MISMATCH, - "Indication SourceInstance is of unexpected type"); - goto out; - } - - prev_inst = data.value.inst; - - out: - return prev_inst; -} -#else static CMPIStatus ActivateFilter(CMPIIndicationMI* mi, const CMPIContext* ctx, const CMPISelectExp* se, @@ -1348,166 +1507,14 @@ static CMPIStatus DeActivateFilter(CMPIIndicationMI* mi, out: return s; } -#endif - -static _EI_RTYPE EnableIndications(CMPIIndicationMI* mi, - const CMPIContext *ctx) -{ - CU_DEBUG("EnableIndications"); - pthread_mutex_lock(&lifecycle_mutex); - lifecycle_enabled = true; - pthread_mutex_unlock(&lifecycle_mutex); - - _EI_RET(); -} - -static _EI_RTYPE DisableIndications(CMPIIndicationMI* mi, - const CMPIContext *ctx) -{ - CU_DEBUG("DisableIndications"); - pthread_mutex_lock(&lifecycle_mutex); - lifecycle_enabled = false; - pthread_mutex_unlock(&lifecycle_mutex); - - _EI_RET(); -} - -DECLARE_FILTER(xen_created, "Xen_ComputerSystemCreatedIndication"); -DECLARE_FILTER(xen_deleted, "Xen_ComputerSystemDeletedIndication"); -DECLARE_FILTER(xen_modified, "Xen_ComputerSystemModifiedIndication"); -DECLARE_FILTER(kvm_created, "KVM_ComputerSystemCreatedIndication"); -DECLARE_FILTER(kvm_deleted, "KVM_ComputerSystemDeletedIndication"); -DECLARE_FILTER(kvm_modified, "KVM_ComputerSystemModifiedIndication"); -DECLARE_FILTER(lxc_created, "LXC_ComputerSystemCreatedIndication"); -DECLARE_FILTER(lxc_deleted, "LXC_ComputerSystemDeletedIndication"); -DECLARE_FILTER(lxc_modified, "LXC_ComputerSystemModifiedIndication"); - -static struct std_ind_filter *filters[] = { - &xen_created, - &xen_deleted, - &xen_modified, - &kvm_created, - &kvm_deleted, - &kvm_modified, - &lxc_created, - &lxc_deleted, - &lxc_modified, - NULL, -}; - -#ifndef USE_LIBVIRT_EVENT -static CMPIStatus raise_indication(const CMPIBroker *broker, - const CMPIContext *ctx, - const CMPIObjectPath *ref, - const CMPIInstance *ind) -{ - CMPIStatus s = {CMPI_RC_OK, NULL}; - CMPIInstance *prev_inst; - CMPIInstance *src_inst; - CMPIObjectPath *_ref = NULL; - struct std_indication_ctx *_ctx = NULL; - struct ind_args *args = NULL; - char *prefix = NULL; - bool rc; - - if (!lifecycle_enabled) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "CSI not enabled, skipping indication delivery"); - goto out; - } - - prev_inst = get_prev_inst(broker, ind, &s); - if (s.rc != CMPI_RC_OK || CMIsNullObject(prev_inst)) { - goto out; - } - - _ref = CMGetObjectPath(prev_inst, &s); - if (s.rc != CMPI_RC_OK) { - cu_statusf(broker, &s, - CMPI_RC_ERR_FAILED, - "Unable to get a reference to the guest"); - goto out; - } - - /* FIXME: This is a Pegasus work around. Pegsus loses the namespace - when an ObjectPath is pulled from an instance */ - if (STREQ(NAMESPACE(_ref), "")) { - CMSetNameSpace(_ref, "root/virt"); - } - - s = get_domain_by_ref(broker, _ref, &src_inst); - if (s.rc != CMPI_RC_OK || CMIsNullObject(src_inst)) { - goto out; - } - - _ctx = malloc(sizeof(struct std_indication_ctx)); - if (_ctx == NULL) { - cu_statusf(broker, &s, - CMPI_RC_ERR_FAILED, - "Unable to allocate indication context"); - goto out; - } - - _ctx->brkr = broker; - _ctx->handler = NULL; - _ctx->filters = filters; - _ctx->enabled = lifecycle_enabled; - - args = malloc(sizeof(struct ind_args)); - if (args == NULL) { - cu_statusf(broker, &s, - CMPI_RC_ERR_FAILED, - "Unable to allocate ind_args"); - goto out; - } - - args->ns = strdup(NAMESPACE(_ref)); - args->classname = strdup(CLASSNAME(_ref)); - if (!args->classname || !args->ns) { - CU_DEBUG("Failed in strdup"); - cu_statusf(broker, &s, - CMPI_RC_ERR_FAILED, - "Failed in strdup in indication raising"); - goto out; - } - args->_ctx = _ctx; - - prefix = class_prefix_name(args->classname); - - rc = _do_indication(broker, ctx, prev_inst, src_inst, - CS_MODIFIED, prefix, args); - - if (!rc) { - cu_statusf(_BROKER, &s, - CMPI_RC_ERR_FAILED, - "Unable to generate indication"); - } - - out: - if (args != NULL) { - stdi_free_ind_args(&args); - } - - if (_ctx != NULL) { - free(_ctx); - } - - free(prefix); - return s; -} -#endif static struct std_indication_handler csi = { -#ifndef USE_LIBVIRT_EVENT - .raise_fn = raise_indication, - .trigger_fn = trigger_indication, -#endif .activate_fn = ActivateFilter, .deactivate_fn = DeActivateFilter, .enable_fn = EnableIndications, .disable_fn = DisableIndications, }; +#endif DEFAULT_IND_CLEANUP(); DEFAULT_AF(); -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This is a pure code move patch. Now codes using libvirt event or native event, are moved into one macro protection, which make code easy to read, and in futher they can be moved into new file as CSI-libvirt.c. This patch also fix code style problem in moved code.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 851 ++++++++++++++++++----------------- 1 files changed, 429 insertions(+), 422 deletions(-)
ACK - lots of trust here as it's not fun looking around for code movement :-) Personally, I prefer interlaced code as it's somewhat easier to maintain long term. Now there's lots duplicated (eg, cut-n-paste) code that's can be the source of future "issues" when one stream is changed but not the other. John

于 2013-3-22 2:33, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This is a pure code move patch. Now codes using libvirt event or native event, are moved into one macro protection, which make code easy to read, and in futher they can be moved into new file as CSI-libvirt.c. This patch also fix code style problem in moved code.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_ComputerSystemIndication.c | 851 ++++++++++++++++++----------------- 1 files changed, 429 insertions(+), 422 deletions(-)
ACK - lots of trust here as it's not fun looking around for code movement :-)
Personally, I prefer interlaced code as it's somewhat easier to maintain long term. Now there's lots duplicated (eg, cut-n-paste) code that's can be the source of future "issues" when one stream is changed but not the other.
Yes, that would be a problem. Duplicated code is not many now(activate/deactite, etc), we can adjust them later.
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

Original code 'continue' in the 'for' in return_enum_vssd() when one VSDS fail in retrieving, but forgot to set s to normal. This patch fix this case. Also many debug message is added to log the error if met. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_VSSD.c | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 975623b..3d2de26 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -181,8 +181,10 @@ static int instance_from_dom(const CMPIBroker *broker, struct domain *dominfo = NULL; ret = get_dominfo(dom, &dominfo); - if (!ret) + if (!ret) { + CU_DEBUG("Failed in get_dominfo()."); goto out; + } op = CMGetObjectPath(inst, NULL); pfx = class_prefix_name(CLASSNAME(op)); @@ -245,6 +247,7 @@ static int instance_from_dom(const CMPIBroker *broker, (dominfo->type == DOMAIN_KVM) || (dominfo->type == DOMAIN_QEMU)) { s = _set_fv_prop(broker, dominfo, inst); if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Failed to set full virtual props."); ret = 0; goto out; } @@ -259,6 +262,7 @@ static int instance_from_dom(const CMPIBroker *broker, dominfo->type); if (asprintf(&vsid, "%s:%s", pfx, dominfo->name) == -1) { + CU_DEBUG("Failed in asprintf()."); ret = 0; goto out; } @@ -328,14 +332,34 @@ static CMPIStatus return_enum_vssd(const CMPIObjectPath *reference, } else if (count == 0) goto out; + int fail_count = 0; for (i = 0; i < count; i++) { CMPIInstance *inst = NULL; inst = _get_vssd(_BROKER, reference, conn, list[i], &s); virDomainFree(list[i]); - if (inst == NULL) + if (inst == NULL) { + /* log the error */ + const char *dom_name = virDomainGetName(list[i]); + if (s.msg) { + CU_DEBUG("Failed to get VSSD instance from " + "domain [%s], status msg [%s].", + dom_name, CMGetCharPtr(s.msg)); + } else { + CU_DEBUG("Failed to get VSSD instance from " + "domain [%s].", + dom_name); + } + /* restore s until last one */ + if (i < count - 1) { + cu_statusf(_BROKER, &s, + CMPI_RC_OK, + "NULL"); + } + fail_count++; continue; + } if (names_only) cu_return_instance_name(results, inst); @@ -343,6 +367,21 @@ static CMPIStatus return_enum_vssd(const CMPIObjectPath *reference, CMReturnInstance(results, inst); } + /* check if some VS fail */ + if (fail_count > 0) { + CU_DEBUG("Failed to get %d VSSD in enum, total is %d.", + fail_count, count); + if (fail_count < count) { + /* consider it succeed, some VSSD will be returned */ + cu_statusf(_BROKER, &s, + CMPI_RC_OK, + "Got %d/%d VSSD, " + "some VS may changed during enum", + count - fail_count, count); + } + } + + out: free(list); virConnectClose(conn); -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Original code 'continue' in the 'for' in return_enum_vssd() when one VSDS fail in retrieving, but forgot to set s to normal. This patch fix this case. Also many debug message is added to log the error if met.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_VSSD.c | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 975623b..3d2de26 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -181,8 +181,10 @@ static int instance_from_dom(const CMPIBroker *broker, struct domain *dominfo = NULL;
ret = get_dominfo(dom, &dominfo); - if (!ret) + if (!ret) { + CU_DEBUG("Failed in get_dominfo()."); goto out; + }
op = CMGetObjectPath(inst, NULL); pfx = class_prefix_name(CLASSNAME(op)); @@ -245,6 +247,7 @@ static int instance_from_dom(const CMPIBroker *broker, (dominfo->type == DOMAIN_KVM) || (dominfo->type == DOMAIN_QEMU)) { s = _set_fv_prop(broker, dominfo, inst); if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Failed to set full virtual props."); ret = 0; goto out; } @@ -259,6 +262,7 @@ static int instance_from_dom(const CMPIBroker *broker, dominfo->type);
if (asprintf(&vsid, "%s:%s", pfx, dominfo->name) == -1) { + CU_DEBUG("Failed in asprintf()."); ret = 0; goto out; } @@ -328,14 +332,34 @@ static CMPIStatus return_enum_vssd(const CMPIObjectPath *reference, } else if (count == 0) goto out;
+ int fail_count = 0;
Put this with the other decls.
for (i = 0; i < count; i++) { CMPIInstance *inst = NULL;
inst = _get_vssd(_BROKER, reference, conn, list[i], &s);
virDomainFree(list[i]); - if (inst == NULL) + if (inst == NULL) { + /* log the error */ + const char *dom_name = virDomainGetName(list[i]);
You'll need to do this before you virDomainFree(list[i]) or reformat the code a bit to do the virDomainFree() twice. Once before the continue below and once if inst != NULL Or do the virDomainFree() below in a for loop before free(list). The rest seems fine. John
+ if (s.msg) { + CU_DEBUG("Failed to get VSSD instance from " + "domain [%s], status msg [%s].", + dom_name, CMGetCharPtr(s.msg)); + } else { + CU_DEBUG("Failed to get VSSD instance from " + "domain [%s].", + dom_name); + } + /* restore s until last one */ + if (i < count - 1) { + cu_statusf(_BROKER, &s, + CMPI_RC_OK, + "NULL"); + } + fail_count++; continue; + }
if (names_only) cu_return_instance_name(results, inst); @@ -343,6 +367,21 @@ static CMPIStatus return_enum_vssd(const CMPIObjectPath *reference, CMReturnInstance(results, inst); }
+ /* check if some VS fail */ + if (fail_count > 0) { + CU_DEBUG("Failed to get %d VSSD in enum, total is %d.", + fail_count, count); + if (fail_count < count) { + /* consider it succeed, some VSSD will be returned */ + cu_statusf(_BROKER, &s, + CMPI_RC_OK, + "Got %d/%d VSSD, " + "some VS may changed during enum", + count - fail_count, count); + } + } + + out: free(list); virConnectClose(conn);

于 2013-3-22 2:43, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
Original code 'continue' in the 'for' in return_enum_vssd() when one VSDS fail in retrieving, but forgot to set s to normal. This patch fix this case. Also many debug message is added to log the error if met.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- src/Virt_VSSD.c | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/src/Virt_VSSD.c b/src/Virt_VSSD.c index 975623b..3d2de26 100644 --- a/src/Virt_VSSD.c +++ b/src/Virt_VSSD.c @@ -181,8 +181,10 @@ static int instance_from_dom(const CMPIBroker *broker, struct domain *dominfo = NULL;
ret = get_dominfo(dom, &dominfo); - if (!ret) + if (!ret) { + CU_DEBUG("Failed in get_dominfo()."); goto out; + }
op = CMGetObjectPath(inst, NULL); pfx = class_prefix_name(CLASSNAME(op)); @@ -245,6 +247,7 @@ static int instance_from_dom(const CMPIBroker *broker, (dominfo->type == DOMAIN_KVM) || (dominfo->type == DOMAIN_QEMU)) { s = _set_fv_prop(broker, dominfo, inst); if (s.rc != CMPI_RC_OK) { + CU_DEBUG("Failed to set full virtual props."); ret = 0; goto out; } @@ -259,6 +262,7 @@ static int instance_from_dom(const CMPIBroker *broker, dominfo->type);
if (asprintf(&vsid, "%s:%s", pfx, dominfo->name) == -1) { + CU_DEBUG("Failed in asprintf()."); ret = 0; goto out; } @@ -328,14 +332,34 @@ static CMPIStatus return_enum_vssd(const CMPIObjectPath *reference, } else if (count == 0) goto out;
+ int fail_count = 0;
Put this with the other decls.
OK.
for (i = 0; i < count; i++) { CMPIInstance *inst = NULL;
inst = _get_vssd(_BROKER, reference, conn, list[i], &s);
virDomainFree(list[i]); - if (inst == NULL) + if (inst == NULL) { + /* log the error */ + const char *dom_name = virDomainGetName(list[i]);
You'll need to do this before you virDomainFree(list[i]) or reformat the code a bit to do the virDomainFree() twice. Once before the continue below and once if inst != NULL
What a stupid error I have, I should be more careful about this. My bad, thank u for the mention!
Or do the virDomainFree() below in a for loop before free(list).
The rest seems fine.
John
+ if (s.msg) { + CU_DEBUG("Failed to get VSSD instance from " + "domain [%s], status msg [%s].", + dom_name, CMGetCharPtr(s.msg)); + } else { + CU_DEBUG("Failed to get VSSD instance from " + "domain [%s].", + dom_name); + } + /* restore s until last one */ + if (i < count - 1) { + cu_statusf(_BROKER, &s, + CMPI_RC_OK, + "NULL"); + } + fail_count++; continue; + }
if (names_only) cu_return_instance_name(results, inst); @@ -343,6 +367,21 @@ static CMPIStatus return_enum_vssd(const CMPIObjectPath *reference, CMReturnInstance(results, inst); }
+ /* check if some VS fail */ + if (fail_count > 0) { + CU_DEBUG("Failed to get %d VSSD in enum, total is %d.", + fail_count, count); + if (fail_count < count) { + /* consider it succeed, some VSSD will be returned */ + cu_statusf(_BROKER, &s, + CMPI_RC_OK, + "Got %d/%d VSSD, " + "some VS may changed during enum", + count - fail_count, count); + } + } + + out: free(list); virConnectClose(conn);
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

This patch adds an common internal function to get property from libvirt-cim config, so it is easy to add properties later. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libxkutil/misc_util.c | 114 +++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 100 insertions(+), 14 deletions(-) diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index f1b93e4..820b42d 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -106,14 +106,40 @@ static const char *cn_to_uri(const char *classname) return NULL; } -static int is_read_only(void) -{ - int readonly = 0; +/* config support */ +typedef enum LibvirtcimConfigType { + CONFIG_BOOL, + CONFIG_STRING, +} LibvirtcimConfigType; + +typedef struct LibvirtcimConfigProperty { + const char *name; + LibvirtcimConfigType value_type; + union { + int value_bool; + char *value_string; + }; + int have_read; +} LibvirtcimConfigProperty; #ifdef HAVE_LIBCONFIG +/* + * @prop must be initialized with zeros, except default value, return 0 means + * OK. prop->value_string will be automatically freed if not NULL, caller must + * never set it to const char*. + */ +static int libvirt_cim_config_get(LibvirtcimConfigProperty *prop) +{ + int error = 0; config_t conf; - int ret; - const char *readonly_str = "readonly"; + int ret = 0; + const char *value_string = NULL; + + /* try only once */ + if (prop->have_read == 1) { + return 0; + } + prop->have_read = 1; config_init(&conf); @@ -121,24 +147,84 @@ static int is_read_only(void) if (ret == CONFIG_FALSE) { CU_DEBUG("Error reading config file at line %d: '%s'\n", conf.error_line, conf.error_text); + error = -1; goto out; } - ret = config_lookup_bool(&conf, readonly_str, &readonly); - if (ret == CONFIG_FALSE) { - CU_DEBUG("'%s' not found in config file, assuming false\n", - readonly_str); - goto out; + switch (prop->value_type) { + case CONFIG_BOOL: + ret = config_lookup_bool(&conf, + prop->name, &prop->value_bool); + if (ret == CONFIG_FALSE) { + CU_DEBUG("Bool property '%s' in config file '%s' " + "not found.", + prop->name, LIBVIRTCIM_CONF); + error = -1; + goto out; + } + + CU_DEBUG("Bool property '%s' in config file '%s' is '%d'.", + prop->name, LIBVIRTCIM_CONF, prop->value_bool); + break; + case CONFIG_STRING: + ret = config_lookup_string(&conf, + prop->name, &value_string); + if (ret == CONFIG_FALSE) { + CU_DEBUG("String property '%s' in config file '%s' " + "not found.", + prop->name, LIBVIRTCIM_CONF); + error = -1; + goto out; + } + + CU_DEBUG("String property '%s' in config file '%s' is '%s'.", + prop->name, LIBVIRTCIM_CONF, value_string); + + if (prop->value_string) { + CU_DEBUG("String property '%s' have value '%s', will " + "be overwritten.", + prop->name, prop->value_string); + free(prop->value_string); + } + prop->value_string = strdup(value_string); + if (!prop->value_string) { + CU_DEBUG("Failed in duplicate value '%s'", + value_string); + error = -1; + goto out; + } + break; + default: + CU_DEBUG("Got invalid property type request %d.", + prop->value_type); + error = -1; + break; } - CU_DEBUG("'%s' value in '%s' config file: %d\n", readonly_str, - LIBVIRTCIM_CONF, readonly); out: config_destroy(&conf); + return error; +} +#else +static int libvirt_cim_config_get(LibvirtcimConfigProperty *prop) +{ + /* try only once */ + if (prop->have_read == 1) { + return 0; + } + prop->have_read = 1; + + CU_DEBUG("Built without libconfig, can't read '%s'.", prop->name); + return -2; +} #endif - /* Default value is 0 (false) */ - return readonly; +static int is_read_only(void) +{ + static LibvirtcimConfigProperty prop = { + "readonly", CONFIG_BOOL, {0}, 0}; + libvirt_cim_config_get(&prop); + return prop.value_bool; } virConnectPtr connect_by_classname(const CMPIBroker *broker, -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This patch adds an common internal function to get property from libvirt-cim config, so it is easy to add properties later.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libxkutil/misc_util.c | 114 +++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 100 insertions(+), 14 deletions(-)
ACK John

This patch allow libvirt-cim to use non-root's ssh key in migration to avoid exposing root's ssh login on server. In some case server are forbidden to expose or provide any root ssh login, and still use ssh encryption between two migration nodes with key of special account created for virtual machine management. When it is enabled in config file: 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key to be copied. It will be copied to [migrate_ssh_temp_key]. 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, use bool property [MigrationWithoutRootKey], to tell whether to use the key as [migrate_ssh_temp_key]. 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be deleted. Details: libvirt-cim would run shell command "cp -f [SSH_Key_Src] [migrate_ssh_temp_key]", then use [migrate_ssh_temp_key] to generate uri suffix for remote connection to migration destination. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libvirt-cim.conf | 19 +++ libxkutil/misc_util.c | 9 ++ libxkutil/misc_util.h | 3 + src/Virt_VSMigrationService.c | 263 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 289 insertions(+), 5 deletions(-) diff --git a/libvirt-cim.conf b/libvirt-cim.conf index d3cb2c3..37d7b0f 100644 --- a/libvirt-cim.conf +++ b/libvirt-cim.conf @@ -11,3 +11,22 @@ # Default value: false # # readonly = false; + +# migrate_ssh_temp_key (string) +# Defines a temp key file which would be used as ssh key in ssh migration, +# Only when it is set, following methods in VirtualSystemMigrationService +# could be used: +# 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key +# to be copied. It will be copied to [migrate_ssh_temp_key]. +# 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, +# use bool property [MigrationWithoutRootKey], to tell whether to use the key +# as [migrate_ssh_temp_key]. +# 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be +# deleted. +# Note: migrate_ssh_temp_key must be set in a directory completely owned by +# root from bottom to top, such as /root/A, or /tmp/A. +# +# Possible values: {any path plus filename on host} +# Default value: NULL, that is not set. +# +# migrate_ssh_temp_key = "/root/vm_migrate_tmp_id_rsa"; diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index 820b42d..00eb4b1 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -227,6 +227,15 @@ static int is_read_only(void) return prop.value_bool; } +const char *get_mig_ssh_tmp_key(void) +{ + static LibvirtcimConfigProperty prop = { + "migrate_ssh_temp_key", CONFIG_STRING, {0}, 0}; + + libvirt_cim_config_get(&prop); + return prop.value_string; +} + virConnectPtr connect_by_classname(const CMPIBroker *broker, const char *classname, CMPIStatus *s) diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h index 90fb2da..0f52290 100644 --- a/libxkutil/misc_util.h +++ b/libxkutil/misc_util.h @@ -152,6 +152,9 @@ int virt_set_status(const CMPIBroker *broker, #define REF2STR(r) CMGetCharPtr(CMObjectPathToString(r, NULL)) +/* get libvirt-cim config */ +const char *get_mig_ssh_tmp_key(void); + /* * Local Variables: * mode: C diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 1f6659d..d03e1a0 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -150,6 +150,7 @@ static CMPIStatus get_migration_uri(CMPIInstance *msd, static char *dest_uri(const char *cn, const char *dest, + const char *dest_params, uint16_t transport) { const char *prefix; @@ -157,6 +158,7 @@ static char *dest_uri(const char *cn, const char *param = ""; char *uri = NULL; int rc; + int param_labeled = 0; if (STARTS_WITH(cn, "Xen")) prefix = "xen"; @@ -197,16 +199,54 @@ static char *dest_uri(const char *cn, goto out; } - if (!STREQC(param, "")) + if (!STREQC(param, "")) { rc = asprintf(&uri, "%s/%s", uri, param); + param_labeled = 1; + } - if (rc == -1) + if (rc == -1) { uri = NULL; + goto out; + } + if (dest_params) { + if (param_labeled == 0) { + rc = asprintf(&uri, "%s?%s", uri, dest_params); + } else { + /* ? is already added */ + rc = asprintf(&uri, "%s%s", uri, dest_params); + } + if (rc == -1) { + uri = NULL; + goto out; + } + } out: return uri; } +/* Todo: move it to libcmpiutil */ +static CMPIrc cu_get_bool_arg_my(const CMPIArgs *args, + const char *name, + bool *target) +{ + CMPIData argdata; + CMPIStatus s; + + argdata = CMGetArg(args, name, &s); + if ((s.rc != CMPI_RC_OK) || CMIsNullValue(argdata)) { + return CMPI_RC_ERR_INVALID_PARAMETER; + } + + if (argdata.type != CMPI_boolean) { + return CMPI_RC_ERR_TYPE_MISMATCH; + } + + *target = (bool)argdata.value.boolean; + + return CMPI_RC_OK; +} + static CMPIStatus get_msd_values(const CMPIObjectPath *ref, const char *destination, const CMPIArgs *argsin, @@ -217,6 +257,13 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, CMPIInstance *msd; uint16_t uri_type; char *uri = NULL; + bool no_root_ssh_key = false; + char *dest_params = NULL; + int ret; + + cu_get_bool_arg_my(argsin, + "MigrationWithoutRootKey", + &no_root_ssh_key); s = get_msd(ref, argsin, &msd); if (s.rc != CMPI_RC_OK) @@ -230,7 +277,27 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, if (s.rc != CMPI_RC_OK) goto out; - uri = dest_uri(CLASSNAME(ref), destination, uri_type); + if (no_root_ssh_key) { + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + CU_DEBUG("Trying migrate with specified ssh key file [%s].", + tmp_keyfile); + ret = asprintf(&dest_params, "keyfile=%s", tmp_keyfile); + if (ret < 0) { + CU_DEBUG("Failed in generating param string."); + goto out; + } + } + + uri = dest_uri(CLASSNAME(ref), destination, dest_params, uri_type); if (uri == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, @@ -238,6 +305,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, goto out; } + CU_DEBUG("Migrate tring to connect remote host with uri %s.", uri); *conn = virConnectOpen(uri); if (*conn == NULL) { CU_DEBUG("Failed to connect to remote host (%s)", uri); @@ -249,7 +317,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, out: free(uri); - + free(dest_params); return s; } @@ -1538,7 +1606,7 @@ static CMPIStatus migrate_vs_host(CMPIMethodMI *self, const char *dhost = NULL; CMPIObjectPath *system; const char *name = NULL; - + cu_get_str_arg(argsin, "DestinationHost", &dhost); cu_get_ref_arg(argsin, "ComputerSystem", &system); @@ -1604,11 +1672,178 @@ static CMPIStatus migrate_vs_system(CMPIMethodMI *self, return migrate_do(ref, ctx, name, dname, argsin, results, argsout); } +/* return 0 on success */ +static int pipe_exec(const char *cmd) +{ + FILE *stream = NULL; + int ret = 0; + char buf[256]; + + CU_DEBUG("executing system cmd [%s].", cmd); + /* Todo: We need a better popen, currently stdout have been closed + and SIGCHILD is handled by tog-pegasus, so fgets always got NULL + making error detection not possible. */ + stream = popen(cmd, "r"); + if (stream == NULL) { + CU_DEBUG("Failed to open pipe to run the command."); + ret = -1; + goto out; + } + usleep(10000); + + buf[255] = 0; + while (fgets(buf, sizeof(buf), stream) != NULL) { + CU_DEBUG("Exception got: [%s].", buf); + ret = -2; + goto out; + } + + out: + if (stream != NULL) { + pclose(stream); + } + return ret; +} + +/* + * libvirt require private key specified to be placed in a directory owned by + * root, because libvirt-cim now runs as root. So here the key would be copied. + * In this way libvirt-cim could borrow a non-root ssh private key, instead of + * using root's private key, avoid security risk. + */ +static int ssh_key_copy(const char *src, const char *dest) +{ + char *cmd = NULL; + int ret = 0; + struct stat sb; + + /* try delete it */ + unlink(dest); + ret = stat(dest, &sb); + if (ret == 0) { + CU_DEBUG("Can not delete [%s] before copy, " + "maybe someone is using it.", + dest); + /* not a fatal fault */ + } + + ret = asprintf(&cmd, "cp -f %s %s", src, dest); + if (ret < 0) { + CU_DEBUG("Failed in combination for shell command."); + goto out; + } + + ret = pipe_exec(cmd); + if (ret < 0) { + CU_DEBUG("Error in executing command [%s]"); + goto out; + } + + ret = stat(dest, &sb); + if (ret < 0) { + CU_DEBUG("Can not find file [%s] after copy.", dest); + } + out: + free(cmd); + return ret; +} + +static CMPIStatus migrate_sshkey_copy(CMPIMethodMI *self, + const CMPIContext *ctx, + const CMPIResult *results, + const CMPIObjectPath *ref, + const CMPIArgs *argsin, + CMPIArgs *argsout) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + const char *ssh_key_src = NULL; + int ret; + + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + + cu_get_str_arg(argsin, "SSH_Key_Src", &ssh_key_src); + if (!ssh_key_src) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Failed to get property 'SSH_Key_Src'."); + CU_DEBUG("Failed to get property 'SSH_Key_Src'."); + goto out; + } + + ret = ssh_key_copy(ssh_key_src, tmp_keyfile); + if (ret < 0) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Got error in copying ssh key from [%s] to [%s].", + ssh_key_src, tmp_keyfile); + CU_DEBUG("Got error in copying ssh key from [%s] to [%s].", + ssh_key_src, tmp_keyfile); + } + + out: + METHOD_RETURN(results, s.rc); + return s; +} + +static CMPIStatus migrate_sshkey_delete(CMPIMethodMI *self, + const CMPIContext *ctx, + const CMPIResult *results, + const CMPIObjectPath *ref, + const CMPIArgs *argsin, + CMPIArgs *argsout) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + int ret; + struct stat sb; + + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + + ret = stat(tmp_keyfile, &sb); + if (ret == 0) { + /* need delete */ + ret = unlink(tmp_keyfile); + if (ret < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to delete [%s].", + tmp_keyfile); + CU_DEBUG("Failed to delete [%s].", tmp_keyfile); + } + } else { + /* not exist */ + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Can not find file [%s] before delete.", + tmp_keyfile); + CU_DEBUG("Can not find file [%s] before delete.", tmp_keyfile); + } + + out: + METHOD_RETURN(results, s.rc); + return s; +}; + static struct method_handler vsimth = { .name = "CheckVirtualSystemIsMigratableToHost", .handler = vs_migratable_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"MigrationWithoutRootKey", CMPI_boolean, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, @@ -1633,6 +1868,7 @@ static struct method_handler mvsth = { .handler = migrate_vs_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"MigrationWithoutRootKey", CMPI_boolean, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, @@ -1652,11 +1888,28 @@ static struct method_handler mvsts = { } }; +static struct method_handler msshkc = { + .name = "MigrateSSHKeyCopy", + .handler = migrate_sshkey_copy, + .args = {{"SSH_Key_Src", CMPI_string, true}, + ARG_END + } +}; + +static struct method_handler msshkd = { + .name = "MigrateSSHKeyDelete", + .handler = migrate_sshkey_delete, + .args = {ARG_END + } +}; + static struct method_handler *my_handlers[] = { &vsimth, &vsimts, &mvsth, &mvsts, + &msshkc, + &msshkd, NULL }; -- 1.7.1

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This patch allow libvirt-cim to use non-root's ssh key in migration to avoid exposing root's ssh login on server. In some case server are forbidden to expose or provide any root ssh login, and still use ssh encryption between two migration nodes with key of special account created for virtual machine management.
When it is enabled in config file: 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key to be copied. It will be copied to [migrate_ssh_temp_key]. 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, use bool property [MigrationWithoutRootKey], to tell whether to use the key as [migrate_ssh_temp_key]. 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be deleted.
Details: libvirt-cim would run shell command "cp -f [SSH_Key_Src] [migrate_ssh_temp_key]", then use [migrate_ssh_temp_key] to generate uri suffix for remote connection to migration destination.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libvirt-cim.conf | 19 +++ libxkutil/misc_util.c | 9 ++ libxkutil/misc_util.h | 3 + src/Virt_VSMigrationService.c | 263 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 289 insertions(+), 5 deletions(-)
Need some more time to look at this - first pass seems OK, but it's late and I'll pick it up again tomorrow. John

On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This patch allow libvirt-cim to use non-root's ssh key in migration to avoid exposing root's ssh login on server. In some case server are forbidden to expose or provide any root ssh login, and still use ssh encryption between two migration nodes with key of special account created for virtual machine management.
When it is enabled in config file: 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key to be copied. It will be copied to [migrate_ssh_temp_key]. 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, use bool property [MigrationWithoutRootKey], to tell whether to use the key as [migrate_ssh_temp_key]. 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be deleted.
Details: libvirt-cim would run shell command "cp -f [SSH_Key_Src] [migrate_ssh_temp_key]", then use [migrate_ssh_temp_key] to generate uri suffix for remote connection to migration destination.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libvirt-cim.conf | 19 +++ libxkutil/misc_util.c | 9 ++ libxkutil/misc_util.h | 3 + src/Virt_VSMigrationService.c | 263 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 289 insertions(+), 5 deletions(-)
diff --git a/libvirt-cim.conf b/libvirt-cim.conf index d3cb2c3..37d7b0f 100644 --- a/libvirt-cim.conf +++ b/libvirt-cim.conf @@ -11,3 +11,22 @@ # Default value: false # # readonly = false; + +# migrate_ssh_temp_key (string) +# Defines a temp key file which would be used as ssh key in ssh migration, +# Only when it is set, following methods in VirtualSystemMigrationService +# could be used: +# 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key +# to be copied. It will be copied to [migrate_ssh_temp_key]. +# 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, +# use bool property [MigrationWithoutRootKey], to tell whether to use the key +# as [migrate_ssh_temp_key]. +# 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be +# deleted. +# Note: migrate_ssh_temp_key must be set in a directory completely owned by +# root from bottom to top, such as /root/A, or /tmp/A. +# +# Possible values: {any path plus filename on host} +# Default value: NULL, that is not set. +# +# migrate_ssh_temp_key = "/root/vm_migrate_tmp_id_rsa"; diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index 820b42d..00eb4b1 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -227,6 +227,15 @@ static int is_read_only(void) return prop.value_bool; }
+const char *get_mig_ssh_tmp_key(void) +{ + static LibvirtcimConfigProperty prop = { + "migrate_ssh_temp_key", CONFIG_STRING, {0}, 0}; + + libvirt_cim_config_get(&prop); + return prop.value_string; +} + virConnectPtr connect_by_classname(const CMPIBroker *broker, const char *classname, CMPIStatus *s) diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h index 90fb2da..0f52290 100644 --- a/libxkutil/misc_util.h +++ b/libxkutil/misc_util.h @@ -152,6 +152,9 @@ int virt_set_status(const CMPIBroker *broker,
#define REF2STR(r) CMGetCharPtr(CMObjectPathToString(r, NULL))
+/* get libvirt-cim config */ +const char *get_mig_ssh_tmp_key(void); + /* * Local Variables: * mode: C diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 1f6659d..d03e1a0 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -150,6 +150,7 @@ static CMPIStatus get_migration_uri(CMPIInstance *msd,
static char *dest_uri(const char *cn, const char *dest, + const char *dest_params, uint16_t transport) { const char *prefix; @@ -157,6 +158,7 @@ static char *dest_uri(const char *cn, const char *param = ""; char *uri = NULL; int rc; + int param_labeled = 0;
if (STARTS_WITH(cn, "Xen")) prefix = "xen"; @@ -197,16 +199,54 @@ static char *dest_uri(const char *cn, goto out; }
- if (!STREQC(param, "")) + if (!STREQC(param, "")) { rc = asprintf(&uri, "%s/%s", uri, param); + param_labeled = 1; + }
- if (rc == -1) + if (rc == -1) { uri = NULL; + goto out; + }
+ if (dest_params) { + if (param_labeled == 0) { + rc = asprintf(&uri, "%s?%s", uri, dest_params); + } else { + /* ? is already added */ + rc = asprintf(&uri, "%s%s", uri, dest_params); + } + if (rc == -1) { + uri = NULL; + goto out; + } + } out: return uri; }
+/* Todo: move it to libcmpiutil */ +static CMPIrc cu_get_bool_arg_my(const CMPIArgs *args, + const char *name, + bool *target) +{ + CMPIData argdata; + CMPIStatus s; + + argdata = CMGetArg(args, name, &s); + if ((s.rc != CMPI_RC_OK) || CMIsNullValue(argdata)) { + return CMPI_RC_ERR_INVALID_PARAMETER; + } + + if (argdata.type != CMPI_boolean) { + return CMPI_RC_ERR_TYPE_MISMATCH; + } + + *target = (bool)argdata.value.boolean; + + return CMPI_RC_OK; +} +
I don't believe the above function is necessary - there is already a cu_get_bool_prop().
static CMPIStatus get_msd_values(const CMPIObjectPath *ref, const char *destination, const CMPIArgs *argsin, @@ -217,6 +257,13 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, CMPIInstance *msd; uint16_t uri_type; char *uri = NULL; + bool no_root_ssh_key = false;
s/no_root_ssh_key/use_non_root_ssh_key/ Just a consideration, not a requirement. When I first saw the variable in use I had to think about it. "no_root_ssh_key" just had a different meaning in my mind.
+ char *dest_params = NULL; + int ret; + + cu_get_bool_arg_my(argsin, + "MigrationWithoutRootKey", + &no_root_ssh_key);
Why not use the existing cu_get_bool_prop(). You don't even check the return value here, so what's the use of your local routine?
s = get_msd(ref, argsin, &msd); if (s.rc != CMPI_RC_OK) @@ -230,7 +277,27 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, if (s.rc != CMPI_RC_OK) goto out;
- uri = dest_uri(CLASSNAME(ref), destination, uri_type); + if (no_root_ssh_key) { + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + CU_DEBUG("Trying migrate with specified ssh key file [%s].", + tmp_keyfile); + ret = asprintf(&dest_params, "keyfile=%s", tmp_keyfile); + if (ret < 0) { + CU_DEBUG("Failed in generating param string."); + goto out; + } + } + + uri = dest_uri(CLASSNAME(ref), destination, dest_params, uri_type); if (uri == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, @@ -238,6 +305,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, goto out; }
+ CU_DEBUG("Migrate tring to connect remote host with uri %s.", uri); *conn = virConnectOpen(uri); if (*conn == NULL) { CU_DEBUG("Failed to connect to remote host (%s)", uri); @@ -249,7 +317,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref,
out: free(uri); - + free(dest_params); return s; }
@@ -1538,7 +1606,7 @@ static CMPIStatus migrate_vs_host(CMPIMethodMI *self, const char *dhost = NULL; CMPIObjectPath *system; const char *name = NULL; - + cu_get_str_arg(argsin, "DestinationHost", &dhost); cu_get_ref_arg(argsin, "ComputerSystem", &system);
@@ -1604,11 +1672,178 @@ static CMPIStatus migrate_vs_system(CMPIMethodMI *self, return migrate_do(ref, ctx, name, dname, argsin, results, argsout); }
+/* return 0 on success */ +static int pipe_exec(const char *cmd) +{ + FILE *stream = NULL; + int ret = 0; + char buf[256]; + + CU_DEBUG("executing system cmd [%s].", cmd); + /* Todo: We need a better popen, currently stdout have been closed + and SIGCHILD is handled by tog-pegasus, so fgets always got NULL + making error detection not possible. */ + stream = popen(cmd, "r"); + if (stream == NULL) { + CU_DEBUG("Failed to open pipe to run the command."); + ret = -1; + goto out; + } + usleep(10000); + + buf[255] = 0; + while (fgets(buf, sizeof(buf), stream) != NULL) { + CU_DEBUG("Exception got: [%s].", buf); + ret = -2; + goto out; + } + + out: + if (stream != NULL) { + pclose(stream); + } + return ret; +} + +/* + * libvirt require private key specified to be placed in a directory owned by + * root, because libvirt-cim now runs as root. So here the key would be copied. + * In this way libvirt-cim could borrow a non-root ssh private key, instead of + * using root's private key, avoid security risk. + */ +static int ssh_key_copy(const char *src, const char *dest) +{ + char *cmd = NULL; + int ret = 0; + struct stat sb; + + /* try delete it */ + unlink(dest); + ret = stat(dest, &sb); + if (ret == 0) { + CU_DEBUG("Can not delete [%s] before copy, " + "maybe someone is using it.", + dest); + /* not a fatal fault */
Maybe not fatal, but what makes you believe a subsequent copy will succeed? Is there a way to keep track of this being "in use" by some other thread that could delete it? It's almost like there needs to be a reference count. Is it possible for two threads to be using it at the same time? Just thinking out loud and trying to consider the negative consequences. In a former job we allowed 1 migration at a time and I'm not aware of the libvirt "restrictions" (yet). IOW, Before we unlink it, if it's already there what does that mean?
+ } + + ret = asprintf(&cmd, "cp -f %s %s", src, dest); + if (ret < 0) { + CU_DEBUG("Failed in combination for shell command."); + goto out; + } + + ret = pipe_exec(cmd); + if (ret < 0) { + CU_DEBUG("Error in executing command [%s]"); + goto out; + } + + ret = stat(dest, &sb); + if (ret < 0) { + CU_DEBUG("Can not find file [%s] after copy.", dest); + } + out: + free(cmd); + return ret; +} + +static CMPIStatus migrate_sshkey_copy(CMPIMethodMI *self, + const CMPIContext *ctx, + const CMPIResult *results, + const CMPIObjectPath *ref, + const CMPIArgs *argsin, + CMPIArgs *argsout) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + const char *ssh_key_src = NULL; + int ret; + + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + + cu_get_str_arg(argsin, "SSH_Key_Src", &ssh_key_src); + if (!ssh_key_src) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Failed to get property 'SSH_Key_Src'."); + CU_DEBUG("Failed to get property 'SSH_Key_Src'."); + goto out; + } + + ret = ssh_key_copy(ssh_key_src, tmp_keyfile); + if (ret < 0) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Got error in copying ssh key from [%s] to [%s].", + ssh_key_src, tmp_keyfile); + CU_DEBUG("Got error in copying ssh key from [%s] to [%s].", + ssh_key_src, tmp_keyfile); + } + + out: + METHOD_RETURN(results, s.rc); + return s; +} + +static CMPIStatus migrate_sshkey_delete(CMPIMethodMI *self, + const CMPIContext *ctx, + const CMPIResult *results, + const CMPIObjectPath *ref, + const CMPIArgs *argsin, + CMPIArgs *argsout) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + int ret; + struct stat sb;
Should we check for "MigrationWithoutRootKey" here too? Is it possible for one thread to request migration without root key while another wants root key? Does the caller really need to know/care? It looks to be an object setting, so we probably do care.
+ + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + + ret = stat(tmp_keyfile, &sb); + if (ret == 0) { + /* need delete */ + ret = unlink(tmp_keyfile); + if (ret < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to delete [%s].", + tmp_keyfile); + CU_DEBUG("Failed to delete [%s].", tmp_keyfile); + } + } else { + /* not exist */ + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Can not find file [%s] before delete.", + tmp_keyfile); + CU_DEBUG("Can not find file [%s] before delete.", tmp_keyfile);
Which perhaps means our other code above did the unlink() for us in another thread? Do you want an error here? John
+ } + + out: + METHOD_RETURN(results, s.rc); + return s; +}; + static struct method_handler vsimth = { .name = "CheckVirtualSystemIsMigratableToHost", .handler = vs_migratable_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"MigrationWithoutRootKey", CMPI_boolean, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, @@ -1633,6 +1868,7 @@ static struct method_handler mvsth = { .handler = migrate_vs_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"MigrationWithoutRootKey", CMPI_boolean, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, @@ -1652,11 +1888,28 @@ static struct method_handler mvsts = { } };
+static struct method_handler msshkc = { + .name = "MigrateSSHKeyCopy", + .handler = migrate_sshkey_copy, + .args = {{"SSH_Key_Src", CMPI_string, true}, + ARG_END + } +}; + +static struct method_handler msshkd = { + .name = "MigrateSSHKeyDelete", + .handler = migrate_sshkey_delete, + .args = {ARG_END + } +}; + static struct method_handler *my_handlers[] = { &vsimth, &vsimts, &mvsth, &mvsts, + &msshkc, + &msshkd, NULL };

于 2013-3-22 21:22, John Ferlan 写道:
On 03/20/2013 11:39 PM, Wenchao Xia wrote:
This patch allow libvirt-cim to use non-root's ssh key in migration to avoid exposing root's ssh login on server. In some case server are forbidden to expose or provide any root ssh login, and still use ssh encryption between two migration nodes with key of special account created for virtual machine management.
When it is enabled in config file: 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key to be copied. It will be copied to [migrate_ssh_temp_key]. 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, use bool property [MigrationWithoutRootKey], to tell whether to use the key as [migrate_ssh_temp_key]. 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be deleted.
Details: libvirt-cim would run shell command "cp -f [SSH_Key_Src] [migrate_ssh_temp_key]", then use [migrate_ssh_temp_key] to generate uri suffix for remote connection to migration destination.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- libvirt-cim.conf | 19 +++ libxkutil/misc_util.c | 9 ++ libxkutil/misc_util.h | 3 + src/Virt_VSMigrationService.c | 263 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 289 insertions(+), 5 deletions(-)
diff --git a/libvirt-cim.conf b/libvirt-cim.conf index d3cb2c3..37d7b0f 100644 --- a/libvirt-cim.conf +++ b/libvirt-cim.conf @@ -11,3 +11,22 @@ # Default value: false # # readonly = false; + +# migrate_ssh_temp_key (string) +# Defines a temp key file which would be used as ssh key in ssh migration, +# Only when it is set, following methods in VirtualSystemMigrationService +# could be used: +# 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key +# to be copied. It will be copied to [migrate_ssh_temp_key]. +# 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, +# use bool property [MigrationWithoutRootKey], to tell whether to use the key +# as [migrate_ssh_temp_key]. +# 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be +# deleted. +# Note: migrate_ssh_temp_key must be set in a directory completely owned by +# root from bottom to top, such as /root/A, or /tmp/A. +# +# Possible values: {any path plus filename on host} +# Default value: NULL, that is not set. +# +# migrate_ssh_temp_key = "/root/vm_migrate_tmp_id_rsa"; diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c index 820b42d..00eb4b1 100644 --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -227,6 +227,15 @@ static int is_read_only(void) return prop.value_bool; }
+const char *get_mig_ssh_tmp_key(void) +{ + static LibvirtcimConfigProperty prop = { + "migrate_ssh_temp_key", CONFIG_STRING, {0}, 0}; + + libvirt_cim_config_get(&prop); + return prop.value_string; +} + virConnectPtr connect_by_classname(const CMPIBroker *broker, const char *classname, CMPIStatus *s) diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h index 90fb2da..0f52290 100644 --- a/libxkutil/misc_util.h +++ b/libxkutil/misc_util.h @@ -152,6 +152,9 @@ int virt_set_status(const CMPIBroker *broker,
#define REF2STR(r) CMGetCharPtr(CMObjectPathToString(r, NULL))
+/* get libvirt-cim config */ +const char *get_mig_ssh_tmp_key(void); + /* * Local Variables: * mode: C diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c index 1f6659d..d03e1a0 100644 --- a/src/Virt_VSMigrationService.c +++ b/src/Virt_VSMigrationService.c @@ -150,6 +150,7 @@ static CMPIStatus get_migration_uri(CMPIInstance *msd,
static char *dest_uri(const char *cn, const char *dest, + const char *dest_params, uint16_t transport) { const char *prefix; @@ -157,6 +158,7 @@ static char *dest_uri(const char *cn, const char *param = ""; char *uri = NULL; int rc; + int param_labeled = 0;
if (STARTS_WITH(cn, "Xen")) prefix = "xen"; @@ -197,16 +199,54 @@ static char *dest_uri(const char *cn, goto out; }
- if (!STREQC(param, "")) + if (!STREQC(param, "")) { rc = asprintf(&uri, "%s/%s", uri, param); + param_labeled = 1; + }
- if (rc == -1) + if (rc == -1) { uri = NULL; + goto out; + }
+ if (dest_params) { + if (param_labeled == 0) { + rc = asprintf(&uri, "%s?%s", uri, dest_params); + } else { + /* ? is already added */ + rc = asprintf(&uri, "%s%s", uri, dest_params); + } + if (rc == -1) { + uri = NULL; + goto out; + } + } out: return uri; }
+/* Todo: move it to libcmpiutil */ +static CMPIrc cu_get_bool_arg_my(const CMPIArgs *args, + const char *name, + bool *target) +{ + CMPIData argdata; + CMPIStatus s; + + argdata = CMGetArg(args, name, &s); + if ((s.rc != CMPI_RC_OK) || CMIsNullValue(argdata)) { + return CMPI_RC_ERR_INVALID_PARAMETER; + } + + if (argdata.type != CMPI_boolean) { + return CMPI_RC_ERR_TYPE_MISMATCH; + } + + *target = (bool)argdata.value.boolean; + + return CMPI_RC_OK; +} +
I don't believe the above function is necessary - there is already a cu_get_bool_prop().
A bit different, this function use const CMPIArgs *args, that one use *inst.
static CMPIStatus get_msd_values(const CMPIObjectPath *ref, const char *destination, const CMPIArgs *argsin, @@ -217,6 +257,13 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, CMPIInstance *msd; uint16_t uri_type; char *uri = NULL; + bool no_root_ssh_key = false;
s/no_root_ssh_key/use_non_root_ssh_key/
Just a consideration, not a requirement. When I first saw the variable in use I had to think about it. "no_root_ssh_key" just had a different meaning in my mind.
OK.
+ char *dest_params = NULL; + int ret; + + cu_get_bool_arg_my(argsin, + "MigrationWithoutRootKey", + &no_root_ssh_key);
Why not use the existing cu_get_bool_prop(). You don't even check the return value here, so what's the use of your local routine?
can't use cu_get_bool_prop(), parameter is different. it have a default value, so no need to check whether fail.
s = get_msd(ref, argsin, &msd); if (s.rc != CMPI_RC_OK) @@ -230,7 +277,27 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, if (s.rc != CMPI_RC_OK) goto out;
- uri = dest_uri(CLASSNAME(ref), destination, uri_type); + if (no_root_ssh_key) { + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + CU_DEBUG("Trying migrate with specified ssh key file [%s].", + tmp_keyfile); + ret = asprintf(&dest_params, "keyfile=%s", tmp_keyfile); + if (ret < 0) { + CU_DEBUG("Failed in generating param string."); + goto out; + } + } + + uri = dest_uri(CLASSNAME(ref), destination, dest_params, uri_type); if (uri == NULL) { cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, @@ -238,6 +305,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, goto out; }
+ CU_DEBUG("Migrate tring to connect remote host with uri %s.", uri); *conn = virConnectOpen(uri); if (*conn == NULL) { CU_DEBUG("Failed to connect to remote host (%s)", uri); @@ -249,7 +317,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref,
out: free(uri); - + free(dest_params); return s; }
@@ -1538,7 +1606,7 @@ static CMPIStatus migrate_vs_host(CMPIMethodMI *self, const char *dhost = NULL; CMPIObjectPath *system; const char *name = NULL; - + cu_get_str_arg(argsin, "DestinationHost", &dhost); cu_get_ref_arg(argsin, "ComputerSystem", &system);
@@ -1604,11 +1672,178 @@ static CMPIStatus migrate_vs_system(CMPIMethodMI *self, return migrate_do(ref, ctx, name, dname, argsin, results, argsout); }
+/* return 0 on success */ +static int pipe_exec(const char *cmd) +{ + FILE *stream = NULL; + int ret = 0; + char buf[256]; + + CU_DEBUG("executing system cmd [%s].", cmd); + /* Todo: We need a better popen, currently stdout have been closed + and SIGCHILD is handled by tog-pegasus, so fgets always got NULL + making error detection not possible. */ + stream = popen(cmd, "r"); + if (stream == NULL) { + CU_DEBUG("Failed to open pipe to run the command."); + ret = -1; + goto out; + } + usleep(10000); + + buf[255] = 0; + while (fgets(buf, sizeof(buf), stream) != NULL) { + CU_DEBUG("Exception got: [%s].", buf); + ret = -2; + goto out; + } + + out: + if (stream != NULL) { + pclose(stream); + } + return ret; +} + +/* + * libvirt require private key specified to be placed in a directory owned by + * root, because libvirt-cim now runs as root. So here the key would be copied. + * In this way libvirt-cim could borrow a non-root ssh private key, instead of + * using root's private key, avoid security risk. + */ +static int ssh_key_copy(const char *src, const char *dest) +{ + char *cmd = NULL; + int ret = 0; + struct stat sb; + + /* try delete it */ + unlink(dest); + ret = stat(dest, &sb); + if (ret == 0) { + CU_DEBUG("Can not delete [%s] before copy, " + "maybe someone is using it.", + dest); + /* not a fatal fault */
Maybe not fatal, but what makes you believe a subsequent copy will
OK, I'll make it fail here as a strict check.
succeed? Is there a way to keep track of this being "in use" by some other thread that could delete it? It's almost like there needs to be a reference count. User can guarentee that no one is using it since all non-root-migrate is started by him and he knows if they have complete or fail.
Is it possible for two threads to be using it at the same time? Just
Yes, possible, and it is normal they share the key.
thinking out loud and trying to consider the negative consequences. In a former job we allowed 1 migration at a time and I'm not aware of the libvirt "restrictions" (yet).
IOW, Before we unlink it, if it's already there what does that mean?
The user just forgot delete it last time, I think it can be deleted and recopy, since there is config file and user knows it is only used by libvirt-cim.
+ } + + ret = asprintf(&cmd, "cp -f %s %s", src, dest); + if (ret < 0) { + CU_DEBUG("Failed in combination for shell command."); + goto out; + } + + ret = pipe_exec(cmd); + if (ret < 0) { + CU_DEBUG("Error in executing command [%s]"); + goto out; + } + + ret = stat(dest, &sb); + if (ret < 0) { + CU_DEBUG("Can not find file [%s] after copy.", dest); + } + out: + free(cmd); + return ret; +} + +static CMPIStatus migrate_sshkey_copy(CMPIMethodMI *self, + const CMPIContext *ctx, + const CMPIResult *results, + const CMPIObjectPath *ref, + const CMPIArgs *argsin, + CMPIArgs *argsout) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + const char *ssh_key_src = NULL; + int ret; + + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + + cu_get_str_arg(argsin, "SSH_Key_Src", &ssh_key_src); + if (!ssh_key_src) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Failed to get property 'SSH_Key_Src'."); + CU_DEBUG("Failed to get property 'SSH_Key_Src'."); + goto out; + } + + ret = ssh_key_copy(ssh_key_src, tmp_keyfile); + if (ret < 0) { + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, + "Got error in copying ssh key from [%s] to [%s].", + ssh_key_src, tmp_keyfile); + CU_DEBUG("Got error in copying ssh key from [%s] to [%s].", + ssh_key_src, tmp_keyfile); + } + + out: + METHOD_RETURN(results, s.rc); + return s; +} + +static CMPIStatus migrate_sshkey_delete(CMPIMethodMI *self, + const CMPIContext *ctx, + const CMPIResult *results, + const CMPIObjectPath *ref, + const CMPIArgs *argsin, + CMPIArgs *argsout) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + int ret; + struct stat sb;
Should we check for "MigrationWithoutRootKey" here too? Is it possible No, this parameter is not needed, it just delete the key, will fail if config is not set.
for one thread to request migration without root key while another wants root key? Does the caller really need to know/care? Yes, it is allowed. This patch just added an option.
It looks to be an
object setting, so we probably do care.
+ + const char *tmp_keyfile = get_mig_ssh_tmp_key(); + if (!tmp_keyfile) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration with special ssh key " + "is not enabled in config file."); + CU_DEBUG("Migration with special ssh key " + "is not enabled in config file."); + goto out; + } + + ret = stat(tmp_keyfile, &sb); + if (ret == 0) { + /* need delete */ + ret = unlink(tmp_keyfile); + if (ret < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Failed to delete [%s].", + tmp_keyfile); + CU_DEBUG("Failed to delete [%s].", tmp_keyfile); + } + } else { + /* not exist */ + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Can not find file [%s] before delete.", + tmp_keyfile); + CU_DEBUG("Can not find file [%s] before delete.", tmp_keyfile);
Which perhaps means our other code above did the unlink() for us in another thread? Do you want an error here? It is possible user called the method twice, so report the error here.
John
+ } + + out: + METHOD_RETURN(results, s.rc); + return s; +}; + static struct method_handler vsimth = { .name = "CheckVirtualSystemIsMigratableToHost", .handler = vs_migratable_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"MigrationWithoutRootKey", CMPI_boolean, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, @@ -1633,6 +1868,7 @@ static struct method_handler mvsth = { .handler = migrate_vs_host, .args = {{"ComputerSystem", CMPI_ref, false}, {"DestinationHost", CMPI_string, false}, + {"MigrationWithoutRootKey", CMPI_boolean, true}, {"MigrationSettingData", CMPI_instance, true}, {"NewSystemSettingData", CMPI_instance, true}, {"NewResourceSettingData", CMPI_instanceA, true}, @@ -1652,11 +1888,28 @@ static struct method_handler mvsts = { } };
+static struct method_handler msshkc = { + .name = "MigrateSSHKeyCopy", + .handler = migrate_sshkey_copy, + .args = {{"SSH_Key_Src", CMPI_string, true}, + ARG_END + } +}; + +static struct method_handler msshkd = { + .name = "MigrateSSHKeyDelete", + .handler = migrate_sshkey_delete, + .args = {ARG_END + } +}; + static struct method_handler *my_handlers[] = { &vsimth, &vsimts, &mvsth, &mvsts, + &msshkc, + &msshkd, NULL };
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

Hi, John These were recoded patches, removing script for bridge patch is removed, please add yours after it. After reconsideration, I think we should stick to libvirt 0.9.0 for libvirt-cim0.6.2, which makes 0.6.2 as steady version and allow user to update to it without any trouble. After it we can raise the libvirt version requirement in config file. Also please add signed-off in your patches. Many thanks.
This serial fix a serial of issues.
V5: General change: Remove script change patch, need John Ferlan's patch in following. 1/15-3/15: new patch make libvirt-cim conform to DSP more. 6/15: better commit message. 8/15: remove a strdup() to reduce a error path. 10/15: free *xml after a fail. 11/15: return fail when ind == NULL in trigger_mod_indiction(), check return value for calloc() in doms_to_xml(), better debug message, check strdup() return on raise_indication, better traverse in dom list. Reduce condition macros. 12/15: new patch to move CSI-libvirt/CSI-libvirt-cim together. 13/15: remove defensive code, only fix the problem of report fail when some VSSD succeed. 14/15: new separate patch from 15/15, to enhance config reading. 15/15: changed the interface, new method to copy/delete the key, rm the key before copy and check the key's status after copy.
Wenchao Xia (15): 1 Remove property CreationClassName in some instance 2 VSSD: add missing property IsFullVirt in schema 3 SDC: use property BootDevices instead of BootDevice 4 do not deregister virt classes in yum upgrade 5 CSI, fix debug print crash 6 CSI, add lock to protect shared data in lifecycle_thread 7 DevicePool, fix debug print crash 8 DevicePool, reimplement get_diskpool_config with libvirt 9 RASDIndication, fix debug print crash 10 device parsing, add debug print 11 CSI Discard libvirt event by default 12 CSI: Move native CSI code together 13 VSSD: report success if not all VS fail in enum 14 misc_util: better way to read config 15 migration: allow ssh based migration with non root's key file
libvirt-cim.conf | 19 + libvirt-cim.spec.in | 12 +- libxkutil/device_parsing.c | 16 +- libxkutil/misc_util.c | 135 +++- libxkutil/misc_util.h | 6 +- schema/Virt_VSSD.mof | 3 + src/Virt_AllocationCapabilities.c | 3 +- src/Virt_ComputerSystem.c | 66 ++- src/Virt_ComputerSystemIndication.c | 1018 +++++++++++++++++--- src/Virt_ConsoleRedirectionService.c | 3 +- src/Virt_ConsoleRedirectionServiceCapabilities.c | 3 +- src/Virt_Device.c | 18 +- src/Virt_DevicePool.c | 198 +++-- src/Virt_EnabledLogicalElementCapabilities.c | 3 +- src/Virt_FilterEntry.c | 3 +- src/Virt_FilterList.c | 3 +- src/Virt_HostSystem.c | 3 +- src/Virt_KVMRedirectionSAP.c | 3 +- src/Virt_RASD.c | 3 +- src/Virt_ReferencedProfile.c | 3 +- src/Virt_RegisteredProfile.c | 3 +- src/Virt_ResourceAllocationSettingDataIndication.c | 6 +- src/Virt_ResourcePoolConfigurationCapabilities.c | 3 +- src/Virt_ResourcePoolConfigurationService.c | 8 +- src/Virt_SettingsDefineCapabilities.c | 10 +- src/Virt_SwitchService.c | 3 +- src/Virt_VSMigrationCapabilities.c | 3 +- src/Virt_VSMigrationService.c | 269 +++++- src/Virt_VSMigrationSettingData.c | 3 +- src/Virt_VSSD.c | 46 +- src/Virt_VirtualSystemManagementCapabilities.c | 3 +- src/Virt_VirtualSystemManagementService.c | 63 ++- src/Virt_VirtualSystemSnapshotService.c | 3 +- ...Virt_VirtualSystemSnapshotServiceCapabilities.c | 3 +- 34 files changed, 1652 insertions(+), 295 deletions(-)
-- Best Regards Wenchao Xia

On 03/20/2013 11:46 PM, Wenchao Xia wrote:
Hi, John These were recoded patches, removing script for bridge patch is removed, please add yours after it. After reconsideration, I think we should stick to libvirt 0.9.0 for libvirt-cim0.6.2, which makes 0.6.2 as steady version and allow user to update to it without any trouble. After it we can raise the libvirt version requirement in config file. Also please add signed-off in your patches.
Many thanks.
I'll repost the bridge patch tomorrow at some point. As for 0.9.0 vs. later - I understand your position, maybe there's some other mechanism that would allow the "choice" for someone to build using a later version that would allow the use of those All API's. What I don't know/understand is when libvirt-cim is built, is a 0.9.0 environment used? Not that it should matter, but I guess I'm just curious. For those patches I had questions/issues with - just post an update and let's take it from there. I don't think you have to repost the whole series unless that's what DV would prefer. I'm not sure what you mean by "add signed-off in your patches". I'm still a bit of a novice w/ git and the process. John

于 2013-3-22 7:16, John Ferlan 写道:
On 03/20/2013 11:46 PM, Wenchao Xia wrote:
Hi, John These were recoded patches, removing script for bridge patch is removed, please add yours after it. After reconsideration, I think we should stick to libvirt 0.9.0 for libvirt-cim0.6.2, which makes 0.6.2 as steady version and allow user to update to it without any trouble. After it we can raise the libvirt version requirement in config file. Also please add signed-off in your patches.
Many thanks.
I'll repost the bridge patch tomorrow at some point.
As for 0.9.0 vs. later - I understand your position, maybe there's some other mechanism that would allow the "choice" for someone to build using a later version that would allow the use of those All API's. What I Possiblely a macro or condition in configure could do it, but I'd like leave the work to be done later.
don't know/understand is when libvirt-cim is built, is a 0.9.0 environment used? Not that it should matter, but I guess I'm just curious. no need to be 0.9.0, my env is 0.9.4 on RH6.3. I remember RH6.2 have a lower libvirt version and some user used it.
For those patches I had questions/issues with - just post an update and let's take it from there. I don't think you have to repost the whole series unless that's what DV would prefer.
I think repost full serial will make patch review/pick easier, we can make sure correct right version will be picked as a complete serial, the changes will be mentioned in the cover-letter so reviewer can skip unchanged patches.
I'm not sure what you mean by "add signed-off in your patches". I'm still a bit of a novice w/ git and the process.
when commit, use command 'git commit -s FILE -m COMMENT' will add this line, which declare the author of it.
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

My series is now posted. I have run both your patches and my patches through cimtest on my f18 box. Although it was not a very enjoyable experience today as recent yum updates and subsequent reboot made things quite frustrating. I couldn't seem to convince the libvirt-cim build to find the libcmpiutil in the 64 bit area. It had something to do with the 'PKG_CONFIG_PATH' variable. Once I worked through that the generation of the rpm was failing because of 'BuildConflicts: sblim-cmpi-devel' in libvirt-cim.spec. My cimtest still has problems with any of the *Indications cimtests - I get errors such as: ComputerSystemIndication - 01_created_indication.py: FAIL Error connecting to CIMOM: [Errno 110] Connection timed out There's a significant pause. Perhaps there's some sort of iptables thing I have to do - I haven't really investigated. I've been able to procure a RHEL6 box and will probably move my testing there (when I have time). John

于 2013-3-23 4:58, John Ferlan 写道:
My series is now posted. I have run both your patches and my patches through cimtest on my f18 box. Although it was not a very enjoyable experience today as recent yum updates and subsequent reboot made things quite frustrating. I couldn't seem to convince the libvirt-cim build to find the libcmpiutil in the 64 bit area. It had something to do with the 'PKG_CONFIG_PATH' variable. Once I worked through that the generation of the rpm was failing because of 'BuildConflicts: sblim-cmpi-devel' in libvirt-cim.spec.
My cimtest still has problems with any of the *Indications cimtests - I get errors such as:
ComputerSystemIndication - 01_created_indication.py: FAIL Error connecting to CIMOM: [Errno 110] Connection timed out
Possible event/indication is not enabled, command "cimconfig -c -l" will list the settings, you can change it by "cimconfig -p -s PROPERTY=VALUE", and then restart tog-pegasus and retry.
There's a significant pause. Perhaps there's some sort of iptables thing I have to do - I haven't really investigated. I've been able to procure a RHEL6 box and will probably move my testing there (when I have time).
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wenchao Xia

On 03/24/2013 03:05 AM, Wenchao Xia wrote:
于 2013-3-23 4:58, John Ferlan 写道:
ComputerSystemIndication - 01_created_indication.py: FAIL Error connecting to CIMOM: [Errno 110] Connection timed out
Possible event/indication is not enabled, command "cimconfig -c -l" will list the settings, you can change it by "cimconfig -p -s PROPERTY=VALUE", and then restart tog-pegasus and retry.
# cimconfig -c -l enableAssociationTraversal=true maxProviderProcesses=0 enableAuditLog=false sslClientVerificationMode=disabled forceProviderProcesses=true idleConnectionTimeout=0 listenAddress=All enableSubscriptionsForNonprivilegedUsers=false slp=false socketWriteTimeout=20 messageDir=msg shutdownTimeout=30 authorizedUserGroups= enableRemotePrivilegedUserAccess=true enableHttpsConnection=true enableIndicationService=true sslCipherSuite=DEFAULT enableNamespaceAuthorization=false sslTrustStoreUserName= maxFailedProviderModuleRestarts=3 enableHttpConnection=true Looks like it is enabled (enableIndicationService) unless I'm missing something. John
participants (2)
-
John Ferlan
-
Wenchao Xia