2:10 a.m.
V2 updates:
1. adjust comments for force use qemu (disable kvm under nested kvm)
2. fix the problem of cdrom (with blank or null disk) VSMS patch caused
in V1
3. fix syntax errors for comments
4. status return updates
5. add description about 8 maximum items limits about vsi support output
Wenchao Xia (1):
make force use qemu configurable
Xu Wang (2):
VSMS: tip error for invalid disk resource
make lldptool command and support output configurable
libvirt-cim.conf | 26 ++++++++++
libxkutil/misc_util.c | 26 ++++++++++
libxkutil/misc_util.h | 3 +
src/Virt_SwitchService.c | 66 ++++++++++++++++++++----
src/Virt_VirtualSystemManagementService.c | 77 ++++++++++++++++++++++-------
5 files changed, 168 insertions(+), 30 deletions(-)
2:10 a.m.
New subject: [PATCH V2 1/3] make force use qemu configurable
From: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
Since in nested KVM, libvirt-cim doesn't handler it well now, so add this
option to manually disable KVM and fall back to qemu. If there is something
fail caused by nested KVM happend, please consider to set this option to be
true for development and testing.
Signed-off-by: Xu Wang <cngesaint(a)outlook.com>
---
libvirt-cim.conf | 10 ++++++++++
libxkutil/misc_util.c | 8 ++++++++
libxkutil/misc_util.h | 1 +
src/Virt_VirtualSystemManagementService.c | 7 +++++++
4 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/libvirt-cim.conf b/libvirt-cim.conf
index 37d7b0f..f378e4c 100644
--- a/libvirt-cim.conf
+++ b/libvirt-cim.conf
@@ -30,3 +30,13 @@
# Default value: NULL, that is not set.
#
# migrate_ssh_temp_key = "/root/vm_migrate_tmp_id_rsa";
+
+# force_use_qemu (bool)
+# Since in nested KVM, libvirt-cim doesn't handler it well now, so add this
+# option to manually disable KVM and fall back to qemu. If there is something
+# fail caused by nested KVM happend, please consider to set this option to be
+# true for development and testing.
+# Possible values: {true,false}
+# Default value: false
+#
+# force_use_qemu = false;
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c
index 00eb4b1..4c0b0a1 100644
--- a/libxkutil/misc_util.c
+++ b/libxkutil/misc_util.c
@@ -227,6 +227,14 @@ static int is_read_only(void)
return prop.value_bool;
}
+bool get_force_use_qemu(void)
+{
+ static LibvirtcimConfigProperty prop = {
+ "force_use_qemu", CONFIG_BOOL, {0}, 0};
+ libvirt_cim_config_get(&prop);
+ return prop.value_bool;
+}
+
const char *get_mig_ssh_tmp_key(void)
{
static LibvirtcimConfigProperty prop = {
diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h
index 0f52290..9e6b419 100644
--- a/libxkutil/misc_util.h
+++ b/libxkutil/misc_util.h
@@ -154,6 +154,7 @@ int virt_set_status(const CMPIBroker *broker,
/* get libvirt-cim config */
const char *get_mig_ssh_tmp_key(void);
+bool get_force_use_qemu(void);
/*
* Local Variables:
diff --git a/src/Virt_VirtualSystemManagementService.c
b/src/Virt_VirtualSystemManagementService.c
index cbb646d..81ec064 100644
--- a/src/Virt_VirtualSystemManagementService.c
+++ b/src/Virt_VirtualSystemManagementService.c
@@ -394,6 +394,13 @@ static bool system_has_kvm(const char *pfx)
virConnectPtr conn;
char *caps = NULL;
bool kvm = false;
+ bool force_use_qemu = get_force_use_qemu();
+
+ /* sometimes disable KVM to avoid problem in nested KVM */
+ if (force_use_qemu) {
+ CU_DEBUG("Enter force use qemu mode!");
+ return false;
+ }
conn = connect_by_classname(_BROKER, pfx, &s);
if ((conn == NULL) || (s.rc != CMPI_RC_OK)) {
--
1.7.1
1:49 p.m.
New subject: [PATCH V2 1/3] make force use qemu configurable
On 05/02/2013 03:10 AM, Xu Wang wrote:
From: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
Since in nested KVM, libvirt-cim doesn't handler it well now, so add this
option to manually disable KVM and fall back to qemu. If there is something
fail caused by nested KVM happend, please consider to set this option to be
true for development and testing.
Signed-off-by: Xu Wang <cngesaint(a)outlook.com>
---
libvirt-cim.conf | 10 ++++++++++
libxkutil/misc_util.c | 8 ++++++++
libxkutil/misc_util.h | 1 +
src/Virt_VirtualSystemManagementService.c | 7 +++++++
4 files changed, 26 insertions(+), 0 deletions(-)
The commit message is still a bit confusing, but I suppose not enough to
hold up adding the capability. Hopefully anyone forced to make changes
as a result of being able to develop/test further in the nested
environment will duly note where and what had to change...
diff --git a/libvirt-cim.conf b/libvirt-cim.conf
index 37d7b0f..f378e4c 100644
--- a/libvirt-cim.conf
+++ b/libvirt-cim.conf
@@ -30,3 +30,13 @@
# Default value: NULL, that is not set.
#
# migrate_ssh_temp_key = "/root/vm_migrate_tmp_id_rsa";
+
+# force_use_qemu (bool)
+# Since in nested KVM, libvirt-cim doesn't handler it well now, so add this
+# option to manually disable KVM and fall back to qemu. If there is something
+# fail caused by nested KVM happend, please consider to set this option to be
+# true for development and testing.
Consider the following explanation to replace the above 4 lines :
When executing in a nested KVM environment libvirt-cim may fail
unexpectedly with a TBS error. Setting "force_use_qemu" option to true
disables use of KVM and falls back to using QEMU directly.
NOTE: TBS should be replaced with the error or the clue that someone
would need in order to determine they should be using/setting this to
true. Remarking about development or testing is irrelevant.
John
+# Possible values: {true,false}
+# Default value: false
+#
+# force_use_qemu = false;
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c
index 00eb4b1..4c0b0a1 100644
--- a/libxkutil/misc_util.c
+++ b/libxkutil/misc_util.c
@@ -227,6 +227,14 @@ static int is_read_only(void)
return prop.value_bool;
}
+bool get_force_use_qemu(void)
+{
+ static LibvirtcimConfigProperty prop = {
+ "force_use_qemu", CONFIG_BOOL, {0},
0};
+ libvirt_cim_config_get(&prop);
+ return prop.value_bool;
+}
+
const char *get_mig_ssh_tmp_key(void)
{
static LibvirtcimConfigProperty prop = {
diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h
index 0f52290..9e6b419 100644
--- a/libxkutil/misc_util.h
+++ b/libxkutil/misc_util.h
@@ -154,6 +154,7 @@ int virt_set_status(const CMPIBroker *broker,
/* get libvirt-cim config */
const char *get_mig_ssh_tmp_key(void);
+bool get_force_use_qemu(void);
/*
* Local Variables:
diff --git a/src/Virt_VirtualSystemManagementService.c
b/src/Virt_VirtualSystemManagementService.c
index cbb646d..81ec064 100644
--- a/src/Virt_VirtualSystemManagementService.c
+++ b/src/Virt_VirtualSystemManagementService.c
@@ -394,6 +394,13 @@ static bool system_has_kvm(const char *pfx)
virConnectPtr conn;
char *caps = NULL;
bool kvm = false;
+ bool force_use_qemu = get_force_use_qemu();
+
+ /* sometimes disable KVM to avoid problem in nested KVM */
+ if (force_use_qemu) {
+ CU_DEBUG("Enter force use qemu mode!");
+ return false;
+ }
conn = connect_by_classname(_BROKER, pfx, &s);
if ((conn == NULL) || (s.rc != CMPI_RC_OK)) {
10:14 p.m.
New subject: [PATCH V2 1/3] make force use qemu configurable
----------------------------------------
Date: Wed, 8 May 2013 14:49:08 -0400
From: jferlan(a)redhat.com
To: libvirt-cim(a)redhat.com
Subject: Re: [Libvirt-cim] [PATCH V2 1/3] make force use qemu configurable
On 05/02/2013 03:10 AM, Xu Wang wrote:
> From: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
>
> Since in nested KVM, libvirt-cim doesn't handler it well now, so add this
> option to manually disable KVM and fall back to qemu. If there is something
> fail caused by nested KVM happend, please consider to set this option to be
> true for development and testing.
>
> Signed-off-by: Xu Wang <cngesaint(a)outlook.com>
> ---
> libvirt-cim.conf | 10 ++++++++++
> libxkutil/misc_util.c | 8 ++++++++
> libxkutil/misc_util.h | 1 +
> src/Virt_VirtualSystemManagementService.c | 7 +++++++
> 4 files changed, 26 insertions(+), 0 deletions(-)
>
The commit message is still a bit confusing, but I suppose not enough to
hold up adding the capability. Hopefully anyone forced to make changes
as a result of being able to develop/test further in the nested
environment will duly note where and what had to change...
There maybe something
unsuitable configuration in the xml files
led to the error under nested KVM. Such as qemu is OK under the same
condition.
> diff --git a/libvirt-cim.conf b/libvirt-cim.conf
> index 37d7b0f..f378e4c 100644
> --- a/libvirt-cim.conf
> +++ b/libvirt-cim.conf
> @@ -30,3 +30,13 @@
> # Default value: NULL, that is not set.
> #
> # migrate_ssh_temp_key = "/root/vm_migrate_tmp_id_rsa";
> +
> +# force_use_qemu (bool)
> +# Since in nested KVM, libvirt-cim doesn't handler it well now, so add this
> +# option to manually disable KVM and fall back to qemu. If there is something
> +# fail caused by nested KVM happend, please consider to set this option to be
> +# true for development and testing.
Consider the following explanation to replace the above 4 lines :
When executing in a nested KVM environment libvirt-cim may fail
unexpectedly with a TBS error. Setting "force_use_qemu" option to true
disables use of KVM and falls back to using QEMU directly.
NOTE: TBS should be replaced with the error or the clue that someone
would need in order to determine they should be using/setting this to
true. Remarking about development or testing is irrelevant.
Thank you very much for
the above words. I think they are much better than
old comment. So the new version patch will use them.
John
> +# Possible values: {true,false}
> +# Default value: false
> +#
> +# force_use_qemu = false;
> diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c
> index 00eb4b1..4c0b0a1 100644
> --- a/libxkutil/misc_util.c
> +++ b/libxkutil/misc_util.c
> @@ -227,6 +227,14 @@ static int is_read_only(void)
> return prop.value_bool;
> }
>
> +bool get_force_use_qemu(void)
> +{
> + static LibvirtcimConfigProperty prop = {
> + "force_use_qemu", CONFIG_BOOL, {0}, 0};
> + libvirt_cim_config_get(&prop);
> + return prop.value_bool;
> +}
> +
> const char *get_mig_ssh_tmp_key(void)
> {
> static LibvirtcimConfigProperty prop = {
> diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h
> index 0f52290..9e6b419 100644
> --- a/libxkutil/misc_util.h
> +++ b/libxkutil/misc_util.h
> @@ -154,6 +154,7 @@ int virt_set_status(const CMPIBroker *broker,
>
> /* get libvirt-cim config */
> const char *get_mig_ssh_tmp_key(void);
> +bool get_force_use_qemu(void);
>
> /*
> * Local Variables:
> diff --git a/src/Virt_VirtualSystemManagementService.c
b/src/Virt_VirtualSystemManagementService.c
> index cbb646d..81ec064 100644
> --- a/src/Virt_VirtualSystemManagementService.c
> +++ b/src/Virt_VirtualSystemManagementService.c
> @@ -394,6 +394,13 @@ static bool system_has_kvm(const char *pfx)
> virConnectPtr conn;
> char *caps = NULL;
> bool kvm = false;
> + bool force_use_qemu = get_force_use_qemu();
> +
> + /* sometimes disable KVM to avoid problem in nested KVM */
> + if (force_use_qemu) {
> + CU_DEBUG("Enter force use qemu mode!");
> + return false;
> + }
>
> conn = connect_by_classname(_BROKER, pfx, &s);
> if ((conn == NULL) || (s.rc != CMPI_RC_OK)) {
>
_______________________________________________
Libvirt-cim mailing list
Libvirt-cim(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvirt-cim
2:10 a.m.
New subject: [PATCH V2 2/3] VSMS: tip error for invalid disk resource
Original code will report xml text missing when a disk is not accessable,
make user confuse. This patch will report the real error to tip user check
its system health state on the server.
Signed-off-by: Xu Wang <cngesaint(a)outlook.com>
---
src/Virt_VirtualSystemManagementService.c | 70 +++++++++++++++++++++--------
1 files changed, 51 insertions(+), 19 deletions(-)
diff --git a/src/Virt_VirtualSystemManagementService.c
b/src/Virt_VirtualSystemManagementService.c
index 81ec064..1652cf2 100644
--- a/src/Virt_VirtualSystemManagementService.c
+++ b/src/Virt_VirtualSystemManagementService.c
@@ -964,11 +964,13 @@ static const char *net_rasd_to_vdev(CMPIInstance *inst,
}
static const char *disk_rasd_to_vdev(CMPIInstance *inst,
- struct virt_device *dev)
+ struct virt_device *dev,
+ char **p_error)
{
const char *val = NULL;
uint16_t type;
bool read = false;
+ int rc;
CU_DEBUG("Enter disk_rasd_to_vdev");
if (cu_get_str_prop(inst, "VirtualDevice", &val) != CMPI_RC_OK)
@@ -984,6 +986,18 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst,
dev->dev.disk.source = strdup(val);
dev->dev.disk.disk_type = disk_type_from_file(val);
+ if ((!XSTREQ(dev->dev.disk.source, "/dev/null")) &&
(dev->dev.disk.disk_type == DISK_UNKNOWN)) {
+ /* on success or fail caller should try free it */
+ rc = asprintf(p_error, "Device %s, Address %s, "
+ "make sure Address can be accessed on host
system.",
+ dev->dev.disk.virtual_dev, dev->dev.disk.source);
+ if (rc == -1) {
+ CU_DEBUG("error during recording exception!");
+ p_error = NULL;
+ }
+ return "Can't get a valid disk type, ";
+ }
+
if (cu_get_u16_prop(inst, "EmulatedType", &type) != CMPI_RC_OK)
type = VIRT_DISK_TYPE_DISK;
@@ -1452,10 +1466,11 @@ static const char *input_rasd_to_vdev(CMPIInstance *inst,
static const char *_sysvirt_rasd_to_vdev(CMPIInstance *inst,
struct virt_device *dev,
uint16_t type,
- const char *ns)
+ const char *ns,
+ char **p_error)
{
if (type == CIM_RES_TYPE_DISK) {
- return disk_rasd_to_vdev(inst, dev);
+ return disk_rasd_to_vdev(inst, dev, p_error);
} else if (type == CIM_RES_TYPE_NET) {
return net_rasd_to_vdev(inst, dev, ns);
} else if (type == CIM_RES_TYPE_MEM) {
@@ -1494,7 +1509,8 @@ static const char *_container_rasd_to_vdev(CMPIInstance *inst,
static const char *rasd_to_vdev(CMPIInstance *inst,
struct domain *domain,
struct virt_device *dev,
- const char *ns)
+ const char *ns,
+ char **p_error)
{
uint16_t type;
CMPIObjectPath *op;
@@ -1516,7 +1532,7 @@ static const char *rasd_to_vdev(CMPIInstance *inst,
if (domain->type == DOMAIN_LXC)
msg = _container_rasd_to_vdev(inst, dev, type, ns);
else
- msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns);
+ msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns, p_error);
out:
if (msg && op)
CU_DEBUG("rasd_to_vdev(%s): %s", CLASSNAME(op), msg);
@@ -1560,7 +1576,8 @@ static char *add_device_nodup(struct virt_device *dev,
static const char *classify_resources(CMPIArray *resources,
const char *ns,
- struct domain *domain)
+ struct domain *domain,
+ char **p_error)
{
int i;
uint16_t type;
@@ -1613,13 +1630,15 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&domain->dev_vcpu[0],
- ns);
+ ns,
+ p_error);
} else if (type == CIM_RES_TYPE_MEM) {
domain->dev_mem_ct = 1;
msg = rasd_to_vdev(inst,
domain,
&domain->dev_mem[0],
- ns);
+ ns,
+ p_error);
} else if (type == CIM_RES_TYPE_DISK) {
struct virt_device dev;
int dcount = count + domain->dev_disk_ct;
@@ -1628,7 +1647,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&dev,
- ns);
+ ns,
+ p_error);
if (msg == NULL)
msg = add_device_nodup(&dev,
domain->dev_disk,
@@ -1646,7 +1666,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&dev,
- ns);
+ ns,
+ p_error);
if (msg == NULL)
msg = add_device_nodup(&dev,
domain->dev_net,
@@ -1676,7 +1697,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&dev,
- ns);
+ ns,
+ p_error);
if (msg == NULL)
msg = add_device_nodup(&dev,
domain->dev_graphics,
@@ -1687,7 +1709,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&domain->dev_input[0],
- ns);
+ ns,
+ p_error);
}
if (msg != NULL)
return msg;
@@ -2083,6 +2106,7 @@ static CMPIInstance *create_system(const CMPIContext *context,
struct inst_list list;
const char *props[] = {NULL};
struct domain *domain = NULL;
+ char *error_msg = NULL;
inst_list_init(&list);
@@ -2113,12 +2137,13 @@ static CMPIInstance *create_system(const CMPIContext *context,
if (s->rc != CMPI_RC_OK)
goto out;
- msg = classify_resources(resources, NAMESPACE(ref), domain);
+ msg = classify_resources(resources, NAMESPACE(ref), domain, &error_msg);
if (msg != NULL) {
- CU_DEBUG("Failed to classify resources: %s", msg);
+ CU_DEBUG("Failed to classify resources: %s, %s",
+ msg, error_msg);
cu_statusf(_BROKER, s,
CMPI_RC_ERR_FAILED,
- "ResourceSettings Error: %s", msg);
+ "ResourceSettings Error: %s, %s", msg, error_msg);
goto out;
}
@@ -2159,6 +2184,7 @@ static CMPIInstance *create_system(const CMPIContext *context,
out:
+ free(error_msg);
cleanup_dominfo(&domain);
free(xml);
inst_list_free(&list);
@@ -2638,6 +2664,7 @@ static CMPIStatus resource_add(struct domain *dominfo,
struct virt_device *dev;
int *count = NULL;
const char *msg = NULL;
+ char *error_msg = NULL;
op = CMGetObjectPath(rasd, &s);
if ((op == NULL) || (s.rc != CMPI_RC_OK))
@@ -2677,12 +2704,12 @@ static CMPIStatus resource_add(struct domain *dominfo,
dev = &list[*count];
dev->type = type;
- msg = rasd_to_vdev(rasd, dominfo, dev, ns);
+ msg = rasd_to_vdev(rasd, dominfo, dev, ns, &error_msg);
if (msg != NULL) {
cu_statusf(_BROKER, &s,
CMPI_RC_ERR_FAILED,
- "Add resource failed: %s",
- msg);
+ "Add resource failed: %s, %s",
+ msg, error_msg);
goto out;
}
@@ -2702,6 +2729,8 @@ static CMPIStatus resource_add(struct domain *dominfo,
(*count)++;
out:
+ free(error_msg);
+
return s;
}
@@ -2718,6 +2747,7 @@ static CMPIStatus resource_mod(struct domain *dominfo,
int *count;
int i;
const char *msg = NULL;
+ char *error_msg = NULL;
CU_DEBUG("Enter resource_mod");
if (devid == NULL) {
@@ -2749,7 +2779,7 @@ static CMPIStatus resource_mod(struct domain *dominfo,
struct virt_device *dev = &list[i];
if (STREQ(dev->id, devid)) {
- msg = rasd_to_vdev(rasd, dominfo, dev, ns);
+ msg = rasd_to_vdev(rasd, dominfo, dev, ns, &error_msg);
if (msg != NULL) {
cu_statusf(_BROKER, &s,
CMPI_RC_ERR_FAILED,
@@ -2793,6 +2823,8 @@ static CMPIStatus resource_mod(struct domain *dominfo,
}
out:
+ free(error_msg);
+
return s;
}
--
1.7.1
1:56 p.m.
New subject: [PATCH V2 2/3] VSMS: tip error for invalid disk resource
On 05/02/2013 03:10 AM, Xu Wang wrote:
Original code will report xml text missing when a disk is not
accessable,
Not resolved from last review:
s/accessable/accessible
make user confuse. This patch will report the real error to tip user
check
its system health state on the server.
I also still see no example of what's being fixed.
Signed-off-by: Xu Wang <cngesaint(a)outlook.com>
---
src/Virt_VirtualSystemManagementService.c | 70 +++++++++++++++++++++--------
1 files changed, 51 insertions(+), 19 deletions(-)
diff --git a/src/Virt_VirtualSystemManagementService.c
b/src/Virt_VirtualSystemManagementService.c
index 81ec064..1652cf2 100644
--- a/src/Virt_VirtualSystemManagementService.c
+++ b/src/Virt_VirtualSystemManagementService.c
@@ -964,11 +964,13 @@ static const char *net_rasd_to_vdev(CMPIInstance *inst,
}veillard(a)redhat.com
static const char *disk_rasd_to_vdev(CMPIInstance *inst,
- struct virt_device *dev)
+ struct virt_device *dev,
+ char **p_error)
{
const char *val = NULL;
uint16_t type;
bool read = false;
+ int rc;
CU_DEBUG("Enter disk_rasd_to_vdev");
if (cu_get_str_prop(inst, "VirtualDevice", &val) != CMPI_RC_OK)
@@ -984,6 +986,18 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst,
dev->dev.disk.source = strdup(val);
dev->dev.disk.disk_type = disk_type_from_file(val);
+ if ((!XSTREQ(dev->dev.disk.source, "/dev/null")) &&
(dev->dev.disk.disk_type == DISK_UNKNOWN)) {
So again, there's no checking for NULL on strdup() for dev.disk.source
which will cause a seg fault here.
NACK and did not review remainder
John
+ /* on success or fail caller should try free it */
+ rc = asprintf(p_error, "Device %s, Address %s, "
+ "make sure Address can be accessed on host
system.",
+ dev->dev.disk.virtual_dev, dev->dev.disk.source);
+ if (rc == -1) {
+ CU_DEBUG("error during recording exception!");
+ p_error = NULL;
+ }
+ return "Can't get a valid disk type, ";
+ }
+
if (cu_get_u16_prop(inst, "EmulatedType", &type) != CMPI_RC_OK)
type = VIRT_DISK_TYPE_DISK;
@@ -1452,10 +1466,11 @@ static const char *input_rasd_to_vdev(CMPIInstance *inst,
static const char *_sysvirt_rasd_to_vdev(CMPIInstance *inst,
struct virt_device *dev,
uint16_t type,
- const char *ns)
+ const char *ns,
+ char **p_error)
{
if (type == CIM_RES_TYPE_DISK) {
- return disk_rasd_to_vdev(inst, dev);
+ return disk_rasd_to_vdev(inst, dev, p_error);
} else if (type == CIM_RES_TYPE_NET) {
return net_rasd_to_vdev(inst, dev, ns);
} else if (type == CIM_RES_TYPE_MEM) {
@@ -1494,7 +1509,8 @@ static const char *_container_rasd_to_vdev(CMPIInstance *inst,
static const char *rasd_to_vdev(CMPIInstance *inst,
struct domain *domain,
struct virt_device *dev,
- const char *ns)
+ const char *ns,
+ char **p_error)
{
uint16_t type;
CMPIObjectPath *op;
@@ -1516,7 +1532,7 @@ static const char *rasd_to_vdev(CMPIInstance *inst,
if (domain->type == DOMAIN_LXC)
msg = _container_rasd_to_vdev(inst, dev, type, ns);
else
- msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns);
+ msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns, p_error);
out:
if (msg && op)
CU_DEBUG("rasd_to_vdev(%s): %s", CLASSNAME(op), msg);
@@ -1560,7 +1576,8 @@ static char *add_device_nodup(struct virt_device *dev,
static const char *classify_resources(CMPIArray *resources,
const char *ns,
- struct domain *domain)
+ struct domain *domain,
+ char **p_error)
{
int i;
uint16_t type;
@@ -1613,13 +1630,15 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&domain->dev_vcpu[0],
- ns);
+ ns,
+ p_error);
} else if (type == CIM_RES_TYPE_MEM) {
domain->dev_mem_ct = 1;
msg = rasd_to_vdev(inst,
domain,
&domain->dev_mem[0],
- ns);
+ ns,
+ p_error);
} else if (type == CIM_RES_TYPE_DISK) {
struct virt_device dev;
int dcount = count + domain->dev_disk_ct;
@@ -1628,7 +1647,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&dev,
- ns);
+ ns,
+ p_error);
if (msg == NULL)
msg = add_device_nodup(&dev,
domain->dev_disk,
@@ -1646,7 +1666,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&dev,
- ns);
+ ns,
+ p_error);
if (msg == NULL)
msg = add_device_nodup(&dev,
domain->dev_net,
@@ -1676,7 +1697,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&dev,
- ns);
+ ns,
+ p_error);
if (msg == NULL)
msg = add_device_nodup(&dev,
domain->dev_graphics,
@@ -1687,7 +1709,8 @@ static const char *classify_resources(CMPIArray *resources,
msg = rasd_to_vdev(inst,
domain,
&domain->dev_input[0],
- ns);
+ ns,
+ p_error);
}
if (msg != NULL)
return msg;
@@ -2083,6 +2106,7 @@ static CMPIInstance *create_system(const CMPIContext *context,
struct inst_list list;
const char *props[] = {NULL};
struct domain *domain = NULL;
+ char *error_msg = NULL;
inst_list_init(&list);
@@ -2113,12 +2137,13 @@ static CMPIInstance *create_system(const CMPIContext *context,
if (s->rc != CMPI_RC_OK)
goto out;
- msg = classify_resources(resources, NAMESPACE(ref), domain);
+ msg = classify_resources(resources, NAMESPACE(ref), domain, &error_msg);
if (msg != NULL) {
- CU_DEBUG("Failed to classify resources: %s", msg);
+ CU_DEBUG("Failed to classify resources: %s, %s",
+ msg, error_msg);
cu_statusf(_BROKER, s,
CMPI_RC_ERR_FAILED,
- "ResourceSettings Error: %s", msg);
+ "ResourceSettings Error: %s, %s", msg, error_msg);
goto out;
}
@@ -2159,6 +2184,7 @@ static CMPIInstance *create_system(const CMPIContext *context,
out:
+ free(error_msg);
cleanup_dominfo(&domain);
free(xml);
inst_list_free(&list);
@@ -2638,6 +2664,7 @@ static CMPIStatus resource_add(struct domain *dominfo,
struct virt_device *dev;
int *count = NULL;
const char *msg = NULL;
+ char *error_msg = NULL;
op = CMGetObjectPath(rasd, &s);
if ((op == NULL) || (s.rc != CMPI_RC_OK))
@@ -2677,12 +2704,12 @@ static CMPIStatus resource_add(struct domain *dominfo,
dev = &list[*count];
dev->type = type;
- msg = rasd_to_vdev(rasd, dominfo, dev, ns);
+ msg = rasd_to_vdev(rasd, dominfo, dev, ns, &error_msg);
if (msg != NULL) {
cu_statusf(_BROKER, &s,
CMPI_RC_ERR_FAILED,
- "Add resource failed: %s",
- msg);
+ "Add resource failed: %s, %s",
+ msg, error_msg);
goto out;
}
@@ -2702,6 +2729,8 @@ static CMPIStatus resource_add(struct domain *dominfo,
(*count)++;
out:
+ free(error_msg);
+
return s;
}
@@ -2718,6 +2747,7 @@ static CMPIStatus resource_mod(struct domain *dominfo,
int *count;
int i;
const char *msg = NULL;
+ char *error_msg = NULL;
CU_DEBUG("Enter resource_mod");
if (devid == NULL) {
@@ -2749,7 +2779,7 @@ static CMPIStatus resource_mod(struct domain *dominfo,
struct virt_device *dev = &list[i];
if (STREQ(dev->id, devid)) {
- msg = rasd_to_vdev(rasd, dominfo, dev, ns);
+ msg = rasd_to_vdev(rasd, dominfo, dev, ns, &error_msg);
if (msg != NULL) {
cu_statusf(_BROKER, &s,
CMPI_RC_ERR_FAILED,
@@ -2793,6 +2823,8 @@ static CMPIStatus resource_mod(struct domain *dominfo,
}
out:
+ free(error_msg);
+
return s;
}
1:01 a.m.
New subject: [PATCH V2 2/3] VSMS: tip error for invalid disk resource
----------------------------------------
Date: Wed, 8 May 2013 14:56:25 -0400
From: jferlan(a)redhat.com
To: libvirt-cim(a)redhat.com
Subject: Re: [Libvirt-cim] [PATCH V2 2/3] VSMS: tip error for invalid disk resource
On 05/02/2013 03:10 AM, Xu Wang wrote:
> Original code will report xml text missing when a disk is not accessable,
Not resolved from last review:
s/accessable/accessible
updated in V3
> make user confuse. This patch will report the real error to tip user check
> its system health state on the server.
I also still see no example of what's being fixed.
Before fixed, if I request a
unavailable disk resource, the output is:
$ wbemexec define_vm.xml
<?xml version="1.0" encoding="utf-8"?>
<CIM CIMVERSION="2.0" DTDVERSION="2.0">
<MESSAGE ID="4711" PROTOCOLVERSION="1.0">
<SIMPLERSP>
<METHODRESPONSE NAME="DefineSystem">
<ERROR CODE="1" DESCRIPTION="CIM_ERR_FAILED: Failed to define domain:
xml
in virDomainDefineXML must not be NULL"/>
</METHODRESPONSE>
</SIMPLERSP>
</MESSAGE>
</CIM>
The output seems to be a xml definition error instead of disk resource unavailable.
Because the error is returned in a wrong place(there is no return if disk resource
request failed).
After fixed, if disk resource is unavailable, the output is:
$ wbemexec define_vm.xml
<?xml version="1.0" encoding="utf-8"?>
<CIM CIMVERSION="2.0" DTDVERSION="2.0">
<MESSAGE ID="4711" PROTOCOLVERSION="1.0">
<SIMPLERSP>
<METHODRESPONSE NAME="DefineSystem">
<ERROR CODE="1" DESCRIPTION="CIM_ERR_FAILED: ResourceSettings Error:
Can't
get a valid disk type, Device vda, Address /var/lib/libvirt/images/test_created_vm2.img,
make sure Address can be accessed on host system."/>
</METHODRESPONSE>
</SIMPLERSP>
</MESSAGE>
</CIM>
So the cause of failure could be printed correctly.
>
> Signed-off-by: Xu Wang <cngesaint(a)outlook.com>
> ---
> src/Virt_VirtualSystemManagementService.c | 70 +++++++++++++++++++++--------
> 1 files changed, 51 insertions(+), 19 deletions(-)
>
> diff --git a/src/Virt_VirtualSystemManagementService.c
b/src/Virt_VirtualSystemManagementService.c
> index 81ec064..1652cf2 100644
> --- a/src/Virt_VirtualSystemManagementService.c
> +++ b/src/Virt_VirtualSystemManagementService.c
> @@ -964,11 +964,13 @@ static const char *net_rasd_to_vdev(CMPIInstance *inst,
> }veillard(a)redhat.com
>
> static const char *disk_rasd_to_vdev(CMPIInstance *inst,
> - struct virt_device *dev)
> + struct virt_device *dev,
> + char **p_error)
> {
> const char *val = NULL;
> uint16_t type;
> bool read = false;
> + int rc;
>
> CU_DEBUG("Enter disk_rasd_to_vdev");
> if (cu_get_str_prop(inst, "VirtualDevice", &val) != CMPI_RC_OK)
> @@ -984,6 +986,18 @@ static const char *disk_rasd_to_vdev(CMPIInstance *inst,
> dev->dev.disk.source = strdup(val);
> dev->dev.disk.disk_type = disk_type_from_file(val);
>
> + if ((!XSTREQ(dev->dev.disk.source, "/dev/null")) &&
(dev->dev.disk.disk_type == DISK_UNKNOWN)) {
So again, there's no checking for NULL on strdup() for dev.disk.source
which will cause a seg fault here.
updated in V3
NACK and did not review remainder
John
> + /* on success or fail caller should try free it */
> + rc = asprintf(p_error, "Device %s, Address %s, "
> + "make sure Address can be accessed on host system.",
> + dev->dev.disk.virtual_dev, dev->dev.disk.source);
> + if (rc == -1) {
> + CU_DEBUG("error during recording exception!");
> + p_error = NULL;
> + }
> + return "Can't get a valid disk type, ";
> + }
> +
> if (cu_get_u16_prop(inst, "EmulatedType", &type) != CMPI_RC_OK)
> type = VIRT_DISK_TYPE_DISK;
>
> @@ -1452,10 +1466,11 @@ static const char *input_rasd_to_vdev(CMPIInstance *inst,
> static const char *_sysvirt_rasd_to_vdev(CMPIInstance *inst,
> struct virt_device *dev,
> uint16_t type,
> - const char *ns)
> + const char *ns,
> + char **p_error)
> {
> if (type == CIM_RES_TYPE_DISK) {
> - return disk_rasd_to_vdev(inst, dev);
> + return disk_rasd_to_vdev(inst, dev, p_error);
> } else if (type == CIM_RES_TYPE_NET) {
> return net_rasd_to_vdev(inst, dev, ns);
> } else if (type == CIM_RES_TYPE_MEM) {
> @@ -1494,7 +1509,8 @@ static const char *_container_rasd_to_vdev(CMPIInstance
*inst,
> static const char *rasd_to_vdev(CMPIInstance *inst,
> struct domain *domain,
> struct virt_device *dev,
> - const char *ns)
> + const char *ns,
> + char **p_error)
> {
> uint16_t type;
> CMPIObjectPath *op;
> @@ -1516,7 +1532,7 @@ static const char *rasd_to_vdev(CMPIInstance *inst,
> if (domain->type == DOMAIN_LXC)
> msg = _container_rasd_to_vdev(inst, dev, type, ns);
> else
> - msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns);
> + msg = _sysvirt_rasd_to_vdev(inst, dev, type, ns, p_error);
> out:
> if (msg && op)
> CU_DEBUG("rasd_to_vdev(%s): %s", CLASSNAME(op), msg);
> @@ -1560,7 +1576,8 @@ static char *add_device_nodup(struct virt_device *dev,
>
> static const char *classify_resources(CMPIArray *resources,
> const char *ns,
> - struct domain *domain)
> + struct domain *domain,
> + char **p_error)
> {
> int i;
> uint16_t type;
> @@ -1613,13 +1630,15 @@ static const char *classify_resources(CMPIArray *resources,
> msg = rasd_to_vdev(inst,
> domain,
> &domain->dev_vcpu[0],
> - ns);
> + ns,
> + p_error);
> } else if (type == CIM_RES_TYPE_MEM) {
> domain->dev_mem_ct = 1;
> msg = rasd_to_vdev(inst,
> domain,
> &domain->dev_mem[0],
> - ns);
> + ns,
> + p_error);
> } else if (type == CIM_RES_TYPE_DISK) {
> struct virt_device dev;
> int dcount = count + domain->dev_disk_ct;
> @@ -1628,7 +1647,8 @@ static const char *classify_resources(CMPIArray *resources,
> msg = rasd_to_vdev(inst,
> domain,
> &dev,
> - ns);
> + ns,
> + p_error);
> if (msg == NULL)
> msg = add_device_nodup(&dev,
> domain->dev_disk,
> @@ -1646,7 +1666,8 @@ static const char *classify_resources(CMPIArray *resources,
> msg = rasd_to_vdev(inst,
> domain,
> &dev,
> - ns);
> + ns,
> + p_error);
> if (msg == NULL)
> msg = add_device_nodup(&dev,
> domain->dev_net,
> @@ -1676,7 +1697,8 @@ static const char *classify_resources(CMPIArray *resources,
> msg = rasd_to_vdev(inst,
> domain,
> &dev,
> - ns);
> + ns,
> + p_error);
> if (msg == NULL)
> msg = add_device_nodup(&dev,
> domain->dev_graphics,
> @@ -1687,7 +1709,8 @@ static const char *classify_resources(CMPIArray *resources,
> msg = rasd_to_vdev(inst,
> domain,
> &domain->dev_input[0],
> - ns);
> + ns,
> + p_error);
> }
> if (msg != NULL)
> return msg;
> @@ -2083,6 +2106,7 @@ static CMPIInstance *create_system(const CMPIContext
*context,
> struct inst_list list;
> const char *props[] = {NULL};
> struct domain *domain = NULL;
> + char *error_msg = NULL;
>
> inst_list_init(&list);
>
> @@ -2113,12 +2137,13 @@ static CMPIInstance *create_system(const CMPIContext
*context,
> if (s->rc != CMPI_RC_OK)
> goto out;
>
> - msg = classify_resources(resources, NAMESPACE(ref), domain);
> + msg = classify_resources(resources, NAMESPACE(ref), domain, &error_msg);
> if (msg != NULL) {
> - CU_DEBUG("Failed to classify resources: %s", msg);
> + CU_DEBUG("Failed to classify resources: %s, %s",
> + msg, error_msg);
> cu_statusf(_BROKER, s,
> CMPI_RC_ERR_FAILED,
> - "ResourceSettings Error: %s", msg);
> + "ResourceSettings Error: %s, %s", msg, error_msg);
> goto out;
> }
>
> @@ -2159,6 +2184,7 @@ static CMPIInstance *create_system(const CMPIContext
*context,
>
>
> out:
> + free(error_msg);
> cleanup_dominfo(&domain);
> free(xml);
> inst_list_free(&list);
> @@ -2638,6 +2664,7 @@ static CMPIStatus resource_add(struct domain *dominfo,
> struct virt_device *dev;
> int *count = NULL;
> const char *msg = NULL;
> + char *error_msg = NULL;
>
> op = CMGetObjectPath(rasd, &s);
> if ((op == NULL) || (s.rc != CMPI_RC_OK))
> @@ -2677,12 +2704,12 @@ static CMPIStatus resource_add(struct domain *dominfo,
> dev = &list[*count];
>
> dev->type = type;
> - msg = rasd_to_vdev(rasd, dominfo, dev, ns);
> + msg = rasd_to_vdev(rasd, dominfo, dev, ns, &error_msg);
> if (msg != NULL) {
> cu_statusf(_BROKER, &s,
> CMPI_RC_ERR_FAILED,
> - "Add resource failed: %s",
> - msg);
> + "Add resource failed: %s, %s",
> + msg, error_msg);
> goto out;
> }
>
> @@ -2702,6 +2729,8 @@ static CMPIStatus resource_add(struct domain *dominfo,
> (*count)++;
>
> out:
> + free(error_msg);
> +
> return s;
> }
>
> @@ -2718,6 +2747,7 @@ static CMPIStatus resource_mod(struct domain *dominfo,
> int *count;
> int i;
> const char *msg = NULL;
> + char *error_msg = NULL;
>
> CU_DEBUG("Enter resource_mod");
> if (devid == NULL) {
> @@ -2749,7 +2779,7 @@ static CMPIStatus resource_mod(struct domain *dominfo,
> struct virt_device *dev = &list[i];
>
> if (STREQ(dev->id, devid)) {
> - msg = rasd_to_vdev(rasd, dominfo, dev, ns);
> + msg = rasd_to_vdev(rasd, dominfo, dev, ns, &error_msg);
> if (msg != NULL) {
> cu_statusf(_BROKER, &s,
> CMPI_RC_ERR_FAILED,
> @@ -2793,6 +2823,8 @@ static CMPIStatus resource_mod(struct domain *dominfo,
> }
>
> out:
> + free(error_msg);
> +
> return s;
> }
>
>
_______________________________________________
Libvirt-cim mailing list
Libvirt-cim(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvirt-cim
2:10 a.m.
New subject: [PATCH V2 3/3] make lldptool command and support output configurable
if lldptool command or vsi support output updates, update items in
the libvirt-cim.conf file. vsi support output could support maximum
8 items.
Signed-off-by: Xu Wang <cngesaint(a)outlook.com>
---
libvirt-cim.conf | 16 +++++++++++
libxkutil/misc_util.c | 18 ++++++++++++
libxkutil/misc_util.h | 2 +
src/Virt_SwitchService.c | 66 ++++++++++++++++++++++++++++++++++++++-------
4 files changed, 91 insertions(+), 11 deletions(-)
diff --git a/libvirt-cim.conf b/libvirt-cim.conf
index f378e4c..0e8f399 100644
--- a/libvirt-cim.conf
+++ b/libvirt-cim.conf
@@ -40,3 +40,19 @@
# Default value: false
#
# force_use_qemu = false;
+
+# lldptool_query_options (string)
+# Defines the command used in SwitchService to query VEPA support, will be
+# used as "lldptool -i [INTERFACE] [OPTIONS]"
+#
+# lldptool_query_options = "-t -g ncb -V evbcfg";
+
+# vsi_support_key_string (string)
+# Defines the string used in SwitchService to search in lldptool's output
+# When lldptool updates its output, please set this value and update the
+# output set, maximum 8 items. use comma to devide items. If there is
+# no need to change the output set, please do not leave it void. Because
+# this value has higher level than default.
+#
+# vsi_support_key_string = "{ supported forwarding mode: (0x40) reflective
relay,"
+# " supported capabilities: (0x7) RTE ECP VDP}";
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c
index 4c0b0a1..6ce8dca 100644
--- a/libxkutil/misc_util.c
+++ b/libxkutil/misc_util.c
@@ -244,6 +244,24 @@ const char *get_mig_ssh_tmp_key(void)
return prop.value_string;
}
+const char *get_lldptool_query_options(void)
+{
+ static LibvirtcimConfigProperty prop = {
+ "lldptool_query_options", CONFIG_STRING, {0}, 0};
+
+ libvirt_cim_config_get(&prop);
+ return prop.value_string;
+}
+
+const char *get_vsi_support_key_string(void)
+{
+ static LibvirtcimConfigProperty prop = {
+ "vsi_support_key_string", 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 9e6b419..4eb588d 100644
--- a/libxkutil/misc_util.h
+++ b/libxkutil/misc_util.h
@@ -155,6 +155,8 @@ int virt_set_status(const CMPIBroker *broker,
/* get libvirt-cim config */
const char *get_mig_ssh_tmp_key(void);
bool get_force_use_qemu(void);
+const char *get_lldptool_query_options(void);
+const char *get_vsi_support_key_string(void);
/*
* Local Variables:
diff --git a/src/Virt_SwitchService.c b/src/Virt_SwitchService.c
index 8991426..983ebc0 100644
--- a/src/Virt_SwitchService.c
+++ b/src/Virt_SwitchService.c
@@ -46,12 +46,29 @@ static CMPIStatus check_vsi_support(char *command)
CMPIStatus s = {CMPI_RC_OK, NULL};
char buff[MAX_LEN];
FILE *stream = NULL;
- const char *searchStr[] = {" supported forwarding mode: "
- "(0x40) reflective relay",
- " supported capabilities: "
- "(0x07) RTE ECP VDP",
- NULL};
- int matched = 0;
+ char *searchStr[8]; /* maximum items of vsi support output */
+ int count = 0;
+ const char *user_settings = get_vsi_support_key_string();
+ char *vsi_support_key_string = NULL;
+ char *delim = "{},";
+ int matched = 0;
+ char *temp = NULL;
+
+ if (!user_settings) {
+ /* default supported output set, 8 maximum */
+ user_settings = "{ supported forwarding mode: "
+ "(0x40) reflective relay,"
+ " supported capabilities: "
+ "(0x7) RTE ECP VDP}";
+ }
+
+ vsi_support_key_string = strdup(user_settings);
+ if (vsi_support_key_string == NULL) {
+ cu_statusf(_BROKER, &s,
+ CMPI_RC_ERR_FAILED,
+ "Strdup vsi_support_key_string failed!");
+ goto out;
+ }
// Run lldptool command to find vsi support.
stream = popen(command, "r");
@@ -63,6 +80,25 @@ static CMPIStatus check_vsi_support(char *command)
goto out;
}
+ /* Slice vsi_support_key_string into items */
+ searchStr[count] = strtok_r(vsi_support_key_string, delim, &temp);
+ if (searchStr[count] == NULL) {
+ CU_DEBUG("searchStr fetch failed when calling strtok_r!");
+ } else {
+ CU_DEBUG("searchStr[%d]: %s", count, searchStr[count]);
+ count++;
+ }
+
+ while ((searchStr[count] = strtok_r(NULL, delim, &temp))) {
+ if (count >= 7) {
+ CU_DEBUG("WARN: searchStr is full, left aborted!");
+ break;
+ } else {
+ CU_DEBUG("searchStr[%d]: %s", count,
searchStr[count]);
+ count++;
+ }
+ }
+
// Read the output of the command.
while (fgets(buff, MAX_LEN, stream) != NULL) {
int i = 0;
@@ -81,16 +117,18 @@ static CMPIStatus check_vsi_support(char *command)
}
/* All the search strings were found in the output of this
command. */
- if (matched == 2) {
+ if (matched == count) {
cu_statusf(_BROKER, &s, CMPI_RC_OK, "VSI
supported");
- goto out;;
+ goto out;
}
}
+
cu_statusf(_BROKER, &s,
CMPI_RC_ERR_NOT_FOUND,
"No VSI Support found");
- out:
+ out:
+ free(vsi_support_key_string);
if (stream != NULL)
pclose(stream);
return s;
@@ -214,6 +252,7 @@ static CMPIStatus get_switchservice(const CMPIObjectPath *reference,
int i;
char **if_list;
char cmd[MAX_LEN];
+ const char *lldptool_query_options = NULL;
*_inst = NULL;
conn = connect_by_classname(broker, CLASSNAME(reference), &s);
@@ -257,10 +296,15 @@ static CMPIStatus get_switchservice(const CMPIObjectPath
*reference,
CU_DEBUG("Found %d interfaces", count);
+ lldptool_query_options = get_lldptool_query_options();
+ if (!lldptool_query_options) {
+ lldptool_query_options = "-t -g ncb -V evbcfg";
+ }
for (i=0; i<count; i++) {
- sprintf(cmd, "lldptool -i %s -t -V evbcfg", if_list[i]);
- CU_DEBUG("running command %s ...", cmd);
+ sprintf(cmd, "lldptool -i %s %s",
+ if_list[i], lldptool_query_options);
+ CU_DEBUG("running command [%s]", cmd);
s = check_vsi_support(cmd);
if (s.rc == CMPI_RC_OK) {
vsi = true;
--
1.7.1
8:03 a.m.
On 05/02/2013 03:10 AM, Xu Wang wrote:
V2 updates:
1. adjust comments for force use qemu (disable kvm under nested kvm)
2. fix the problem of cdrom (with blank or null disk) VSMS patch caused
in V1
3. fix syntax errors for comments
4. status return updates
5. add description about 8 maximum items limits about vsi support output
Wenchao Xia (1):
make force use qemu configurable
Xu Wang (2):
VSMS: tip error for invalid disk resource
make lldptool command and support output configurable
libvirt-cim.conf | 26 ++++++++++
libxkutil/misc_util.c | 26 ++++++++++
libxkutil/misc_util.h | 3 +
src/Virt_SwitchService.c | 66 ++++++++++++++++++++----
src/Virt_VirtualSystemManagementService.c | 77 ++++++++++++++++++++++-------
5 files changed, 168 insertions(+), 30 deletions(-)
Very strange - this is the 2nd time patch 3/3 didn't make it to my email
even though it's on the list. In fact my reply nor either of your
responses to my reply made it to the mail server. It's as if it has some
magic word or setting so that the mail server refuses to download it.
In any case, I'm still not convinced there's not a memory leak, but
perhaps a Coverity or Valgrind run would answer that. I'll try to set up
Coverity run on the sources.
Another consideration - perhaps the get_lldptool_query_options() and
get_vsi_support_key_string() should return a char * of the strdup()'d
prop.value string. You may also want to consider putting the defaults
into those functions too to hide them from the caller.
For example, instead of:
+const char *get_lldptool_query_options(void)
+{
+ static LibvirtcimConfigProperty prop = {
+ "lldptool_query_options", CONFIG_STRING, {0}, 0};
+
+ libvirt_cim_config_get(&prop);
+ return prop.value_string;
+}
+
go with
char *get_lldptool_query_options(void)
{
static LibvirtcimConfigProperty prop = {
"lldptool_query_options", CONFIG_STRING, {0}, 0};
libvirt_cim_config_get(&prop);
if (prop.value_string)
return strdup(prop.value_string);
return strdup("-t -g ncb -V evbcfg");
}
The caller checks for NULL return, errors appropriately and can free()
the returned string.
John
1:47 a.m.
----------------------------------------
Date: Thu, 9 May 2013 09:03:20 -0400
From: jferlan(a)redhat.com
To: libvirt-cim(a)redhat.com
Subject: Re: [Libvirt-cim] [PATCH V2 0/3] libvirt-cim patches
On 05/02/2013 03:10 AM, Xu Wang wrote:
> V2 updates:
> 1. adjust comments for force use qemu (disable kvm under nested kvm)
> 2. fix the problem of cdrom (with blank or null disk) VSMS patch caused
> in V1
> 3. fix syntax errors for comments
> 4. status return updates
> 5. add description about 8 maximum items limits about vsi support output
>
>
> Wenchao Xia (1):
> make force use qemu configurable
>
> Xu Wang (2):
> VSMS: tip error for invalid disk resource
> make lldptool command and support output configurable
>
> libvirt-cim.conf | 26 ++++++++++
> libxkutil/misc_util.c | 26 ++++++++++
> libxkutil/misc_util.h | 3 +
> src/Virt_SwitchService.c | 66 ++++++++++++++++++++----
> src/Virt_VirtualSystemManagementService.c | 77 ++++++++++++++++++++++-------
> 5 files changed, 168 insertions(+), 30 deletions(-)
>
>
Very strange - this is the 2nd time patch 3/3 didn't make it to my email
even though it's on the list. In fact my reply nor either of your
responses to my reply made it to the mail server. It's as if it has some
magic word or setting so that the mail server refuses to download it.
I'll Cc
you in every mail:-)
In any case, I'm still not convinced there's not a memory leak, but
perhaps a Coverity or Valgrind run would answer that. I'll try to set up
Coverity run on the sources.
It's a static variable so every caller use the
same memory space. It needn't
released by me and shouldn't be freed because other caller may use it too.
Another consideration - perhaps the get_lldptool_query_options() and
get_vsi_support_key_string() should return a char * of the strdup()'d
prop.value string. You may also want to consider putting the defaults
into those functions too to hide them from the caller.
For example, instead of:
+const char *get_lldptool_query_options(void)
+{
+ static LibvirtcimConfigProperty prop = {
+ "lldptool_query_options", CONFIG_STRING, {0}, 0};
+
+ libvirt_cim_config_get(&prop);
+ return prop.value_string;
+}
+
go with
char *get_lldptool_query_options(void)
{
static LibvirtcimConfigProperty prop = {
"lldptool_query_options", CONFIG_STRING, {0}, 0};
libvirt_cim_config_get(&prop);
if (prop.value_string)
return strdup(prop.value_string);
return strdup("-t -g ncb -V evbcfg");
}
The caller checks for NULL return, errors appropriately and can free()
the returned string.
Yes, it maybe a feasible way to deal with this command. I
discussed with Wenchao
for our design ideas and at last we decided to use the old code. The reason is that
we think get_xxx method just include fetch value of pointed keywords string and return
it to the caller. It wouldn't contain any logic processing. We'll keep the coding
style.
John
_______________________________________________
Libvirt-cim mailing list
Libvirt-cim(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvirt-cim
4445
days inactive
4456
days old
9 comments
3 participants
participants (3)
-
John Ferlan -
WangXu -
Xu Wang