[Libvirt-cim] [PATCH] Changed the output of AllocationCapability association when using DiskPool as input

[Kaitlin Rupert]

> @@ -638,7 +590,7 @@
>                        (CMPIValue *)"MegaBytes", CMPI_chars);
>          CMSetProperty(inst, "VirtualQuantity",
>                        (CMPIValue *)&disk_size, CMPI_uint64);


Instead of setting the vol_size as 0, just don't set the VirtualQuantity 
attribute for CDROM instances.

> -        CMSetProperty(inst, "Address", (CMPIValue *)addr, CMPI_chars);
> +        CMSetProperty(inst, "Address", (CMPIValue *)disk_path, CMPI_chars);
> 
>          if (type == DOMAIN_LXC)
>                  CMSetProperty(inst, "MountPoint", (CMPIValue *)dev, CMPI_chars);
> @@ -664,34 +616,28 @@
>          return s;
>  }
> 
> -static CMPIStatus disk_template(const CMPIObjectPath *ref,
> -                                int template_type,
> -                                struct inst_list *list)
> +static CMPIStatus cdrom_template(const CMPIObjectPath *ref,
> +                                  int template_type,
> +                                  struct inst_list *list)
>  {
> -        bool ret;
>          char *pfx;
>          const char *id;
> -        uint64_t disk_size;
> -        uint16_t emu_type = 0;
> +        char *vol_path;
> +        uint64_t vol_size = 0;
>          CMPIStatus s = {CMPI_RC_OK, NULL};
> +        uint16_t emu_type = 1;
> 
>          switch(template_type) {
>          case SDC_RASD_MIN:
> -                disk_size = SDC_DISK_MIN;
>                  id = "Minimum";

To distinguish these instances from other instances, can you use 
something like "Minimum CDROM" or something similar?

> +
> +static CMPIStatus volume_template(const CMPIObjectPath *ref,
> +                                  int template_type,
> +                                  virStorageVolPtr volume_ptr,
> +                                  struct inst_list *list)
> +{
> +        char *pfx;
> +        const char *id;
> +        char *vol_path;
> +        uint64_t vol_size;
> +        virStorageVolInfo vol_info;
> +        CMPIStatus s = {CMPI_RC_OK, NULL};
> +        int retval;
> +        uint16_t emu_type = 0;
> +
> +        retval = virStorageVolGetInfo(volume_ptr, &vol_info);

Most of the rest of the code uses "ret" for this.

> +        if (retval == -1) {
> +                virt_set_status(_BROKER, &s,
> +                                CMPI_RC_ERR_FAILED,
> +                                virStorageVolGetConnect(volume_ptr),
> +                                "Unable to get volume information");
> +                goto out;
> +        }
> +        
> +        switch(template_type) {
> +        case SDC_RASD_MIN:
> +                vol_size = SDC_DISK_MIN;

If SDC_DISK_MIN is greater than vol_info.capacity, vol_info.capacity 
should be used instead.


> +        } else if (STREQ(pfx, "KVM")) {
>                  s = set_disk_props(DOMAIN_KVM,
>                                     ref, 
> -                                   id, 
> -                                   disk_size, 
> +                                   id,
> +                                   vol_path, 
> +                                   vol_size, 
>                                     emu_type, 
>                                     list); 
>          } else if (STREQ(pfx, "LXC")) {
>                  s = set_disk_props(DOMAIN_LXC,
>                                     ref, 
>                                     id, 
> -                                   disk_size, 
> +                                   vol_path,

Containers guests don't have a disk associated with them.  Instead, the 
Address attribute points to a directory that is used as the root 
directory for the guest.

So you don't want to generate an LXC guest for each volume in the 
diskpool.  And you'll want to use a dummy directory (like "/tmp") for 
the Address.

> +                                   vol_size, 
>                                     emu_type, 
>                                     list); 
>          } else {
> @@ -762,6 +782,109 @@
>          return s;
>  }
> 
> +static CMPIStatus disk_template(const CMPIObjectPath *ref,
> +                                int template_type,
> +                                struct inst_list *list)
> +{
> +        CMPIStatus s = {CMPI_RC_OK, NULL};
> +        virConnectPtr conn = NULL;
> +        virStoragePoolPtr poolptr = NULL;
> +        virStorageVolPtr volptr = NULL;
> +        const char *instid = NULL;
> +        char *host = NULL;
> +        const char *poolname = NULL;
> +        char **volnames = NULL;
> +        int numvols, numvolsret = 0;

Declare these on separate lines.

> +        int i;
> +
> +
> +        conn = connect_by_classname(_BROKER, CLASSNAME(ref), &s);
> +
> +        if (cu_get_str_path(ref, "InstanceID", &instid) != CMPI_RC_OK) {
> +               cu_statusf(_BROKER, &s,
> +                          CMPI_RC_ERR_FAILED,
> +                          "Unable to get InstanceID for disk device");
> +               goto out;
> +        }
> +
> +        if (parse_fq_devid(instid, &host, (char **)&poolname) != 1) {

Passing a variable in this manner is pretty ugly.  Instead, declare 
poolname as a char (and be sure to free it when you're done).  The other 
providers do this as well, so it's good to be consistent when possible.

> +                cu_statusf(_BROKER, &s, 
> +                           CMPI_RC_ERR_FAILED,
> +                           "Unable to get pool device id");
> +                goto out;
> +        }
> +
> +        if ((poolptr = virStoragePoolLookupByName(conn, poolname)) == NULL) {

Since virStoragePoolLookupByName(), use another variable and strdup() 
poolname.  I know it's a few extra lines of code, but it's cleaner in my 
opinion.

> +
> +
> +        volnames = (char **)malloc(sizeof(char *) * numvols);

Verify volnames was allocated appropriately.  Return an error if it isn't.

> +
> +        numvolsret = virStoragePoolListVolumes(poolptr, 
> +                                               volnames, 
> +                                               numvols);

You can fit all the params on one line if you'd like.

> +
> +        if (numvolsret == -1) {
> +                virt_set_status(_BROKER, &s,
> +                                CMPI_RC_ERR_FAILED,
> +                                conn,
> +                                "Unable to get a pointer to volumes \
> +                                of storage pool `%s'",
> +                                poolname);
> +                goto out;
> +        }
> +        
> +        for (i = 0; i < numvolsret; i++) {
> +                

No space is needed here.

> +                volptr = virStorageVolLookupByName(poolptr, volnames[i]);
> +                if (volptr == NULL) {
> +                        virt_set_status(_BROKER, &s,
> +                                        CMPI_RC_ERR_NOT_FOUND,
> +                                        conn,
> +                                        "Storage Volume `%s' not found",
> +                                        volnames[i]);
> +                        goto out;
> +                }         
> +                
> +                s = volume_template(ref,
> +                                    template_type,
> +                                    volptr,
> +                                    list);

Same here, all the params can fit on one line.  We usually only break up 
a line if it spans 80 characters.

> +                if (s.rc != CMPI_RC_OK)
> +                        goto out;            
> +        }
> +
> +        s = cdrom_template(ref,
> +                           template_type,
> +                           list);
> +
> + out:
> +        if(volnames != NULL)

No need for the if here.  free() will check to see if the variable is NULL.

-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com