Re: [Libvirt-cim] [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

> 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&gt; > --- > 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; > } > > _______________________________________________ Libvirt-cim mailing list Libvirt-cim(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim