[Libvirt-cim] [PATCH V2 2/3] VSMS: tip error for invalid disk resource

[WangXu]

----------------------------------------
> Date: Wed, 8 May 2013 14:56:25 -0400
> From: jferlan at redhat.com
> To: libvirt-cim at 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 at 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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim