[Libvirt-cim] [PATCH 2/3] VSMS: tip error for invalid disk resource
WangXu
cngesaint at outlook.com
Sat Apr 27 09:38:41 UTC 2013
> Date: Thu, 25 Apr 2013 13:05:51 -0400
> From: jferlan at redhat.com
> To: libvirt-cim at redhat.com
> Subject: Re: [Libvirt-cim] [PATCH 2/3] VSMS: tip error for invalid disk resource
>
> 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...
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 <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...
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],
> > - 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...
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
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvirt-cim/attachments/20130427/878f5202/attachment.htm>
More information about the Libvirt-cim
mailing list