[Libvirt-cim] [PATCH 2/3] VSMS: tip error for invalid disk resource
John Ferlan
jferlan at redhat.com
Thu Apr 25 17:05:51 UTC 2013
On 04/23/2013 05:30 AM, cngesaint at outlook.com wrote:
> From: Xu Wang <cngesaint at outlook.com>
>
> 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
s/accessable/accessible/
> its system health state on the server.
Can you provide an example test or command - so that it's "testable"?
Whether that's by adding a new cimtest or some other means. There seems
to be two errors serviced by DISK_UNKNOWN - the first one is a failure
on a 'stat64()' and the second is the st_mode not being a BLK device, a
REG (file), or a DIR (file system). How are they differentiated?
Seems to me earlier checks should determine that the path doesn't exist
while this check should be limited to invalid format. My other
experience with CIM enum's is that there's supposed to be an "UNKNOWN"
and "OTHER" values, where UNKNOWN was always 0 and OTHER was always 1.
I may be the "OTHER" name space incorrect it's been a while...
>
> Signed-off-by: Xu Wang <cngesaint at outlook.com>
> ---
> src/Virt_VirtualSystemManagementService.c | 65 +++++++++++++++++++++--------
> 1 files changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c
> index 4e93ef0..d252e12 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,17 @@ 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 (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);
There's no error checking on whether the strdup()'s succeeded and thus
this could cause problems with %s and NULL strings. For that matter
there's very little error checking w/r/t strdup() failures so you're
following existing potential issues...
> + if (rc == -1) {
> + CU_DEBUG("error during recording exception!");
Since asprintf() says the parameter 1 is "undefined" if rc == -1, so be
safe and set p_error to NULL again...
> + }
> + return "Can't get a valid disk type, ";
Looks like a cut-n-paste - just snip the ", ". Other error returns
don't use the ", " list marker...
> + }
> +
> if (cu_get_u16_prop(inst, "EmulatedType", &type) != CMPI_RC_OK)
> type = VIRT_DISK_TYPE_DISK;
>
> @@ -1452,10 +1465,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 +1508,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 +1531,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 +1575,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 +1629,15 @@ static const char *classify_resources(CMPIArray *resources,
> msg = rasd_to_vdev(inst,
> domain,
> &domain->dev_vcpu[0],
> - veillard at redhat.com ns);
> + ns,
> + p_error);
> } else if (type == CIM_RES_TYPE_MEM) {
> domain->dev_mem_ct = 1;
> msg = rasd_to_vdevvirQEMUDriverCreateCapabilities(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 +1646,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 +1665,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 +1696,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 +1708,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 +2105,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 +2136,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);
Since error_msg could be NULL - it should be handled...
> cu_statusf(_BROKER, s,
> CMPI_RC_ERR_FAILED,
> - "ResourceSettings Error: %s", msg);
> + "ResourceSettings Error: %s, %s", msg, error_msg);
Same here...
> goto out;
> }
>
> @@ -2159,6 +2183,7 @@ static CMPIInstance *create_system(const CMPIContext *context,
>
>
> out:
> + free(error_msg);
> cleanup_dominfo(&domain);
> free(xml);
> inst_list_free(&list);
> @@ -2638,6 +2663,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,7 +2703,7 @@ 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,
Why not add the "error_msg" output here too like create_system?
> @@ -2702,6 +2728,8 @@ static CMPIStatus resource_add(struct domain *dominfo,
> (*count)++;
>
> out:
> + free(error_msg);
> +virQEMUDriverCreateCapabilities
> return s;
> }
>
> @@ -2718,6 +2746,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 +2778,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,
Same comment
John
> @@ -2793,6 +2822,8 @@ static CMPIStatus resource_mod(struct domain *dominfo,
> }
>
> out:
> + free(error_msg);
> +
> return s;
> }
>
>
More information about the Libvirt-cim
mailing list