> Date: Thu, 25 Apr 2013 13:05:51 -0400 > From: [email protected] > To: [email protected] > Subject: Re: [Libvirt-cim] [PATCH 2/3] VSMS: tip error for invalid disk > resource > > On 04/23/2013 05:30 AM, [email protected] wrote: > > From: Xu Wang <[email protected]> > > > > 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... Yes, I just found it is not suitable for just checking resource accessible by disk type. e.g.,under cdrom (blank or none disk) this patch will trigger another question. So I will rewrite it. > > > > > > Signed-off-by: Xu Wang <[email protected]> > > --- > > 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... Yes, I think so. > > > + } > > + 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... It's caused by my input method, sorry... > > > > + } > > + > > 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], > > - [email protected] 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... It would be OK. If error_msg is null, a blank will output and no error. > > > 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? Yes, error_msg also could be returned to server from here:-) > > > @@ -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; > > } > > > > > > _______________________________________________ > Libvirt-cim mailing list > [email protected] > https://www.redhat.com/mailman/listinfo/libvirt-cim
_______________________________________________ Libvirt-cim mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvirt-cim
