Re: [libvirt] [PATCH 1/2] Addition of XenAPI support to libvirt

2010/2/26 Sharadha Prabhakar (3P) <sharadha.prabhakar(a)citrix.com&gt;: You should move this into the xenapiPrivate struct, otherwise you'll have trouble using multiple XenAPI connections at the same time, because multiple calls to xenapiOpen will overwrite the pointer an leak the previous value. If the URI is not for you (scheme != XenAPI) then you must decline it (return VIR_DRV_OPEN_DECLINED) and not return VIR_DRV_OPEN_ERROR. All other schemes in libvirt are lowercase. I suggest you change it to "xenapi" or switch the STREQ to STRCASEEQ to do a case-insensitive comparison. You cannot use ESX driver code here, because inter-driver dependencies are forbidden. So we could either move this function into the src/utils/utils.c or you duplicate the function in the XenAPI driver code. I suggest the later because esxUtil_RequestPassword's behavior is special for the ESX driver (setting the challenge to the hostname). You should check the return value from virAsprintf and report an OOM error if the return value is negative. I'm not sure if it's a good idea to change global libxml2 defaults. I can't tell if this is the default anyway, or if it'll will affect the other XML parsing code in libvirt in a negative way, because the libxml2 documentation for this function is confusing to me. You need to call virReportOOMError() here, to report the allocation failure. Remove ATTRIBUTE_UNUSED from conn, as conn is used in this function. You're abusing the VIR_ERR_NO_DEVICE error code here. You should define new variables always at the beginning of a block. You need to check the return value strdup of for NULL and report an OOM error using virReportOOMError if it's NULL. Also you need to break the for loop if you found the version string, otherwise you may strdup and assign a string multiple times to version and leak the previous allocated memeory. You should check the return value of sscanf to see if it failed. You're abusing the VIR_ERR_NO_DEVICE error code here. You should use virStrncpy here and check its return value. You're not supposed to build the caps structure by hand. Use the virCapabilities* functions. Use virCapabilitiesNew instead. Use virCapabilitiesAddGuest and virCapabilitiesAddGuestDomain instead. You could improve this by creating the caps once per connection and store the virCapsPtr in the xenapiPrivate struct, instead of recreating it over and over again. You're abusing the VIR_ERR_NO_DEVICE error code here. That's wrong. Assume the case that result->size is greater than maxids, then you are reporting back more IDs than you actually wrote to the ids array. You're abusing the VIR_ERR_NO_DEVICE error code here. createVMRecordFromXml doesn't copy the UUID from the virDomainDefPtr to the xen_vm_record, so parsing it here gives wrong results. You forgot to set the domain ID (domP->id). You're leaking the caps object here. You should break the for loop if the domain id found. You should report an error if the domain is not found. You're abusing the VIR_ERR_NO_DEVICE error code here. const unsigned char *uuid is already in raw format. Parsing it again will give wrong results. Did you actually test this function? The domain ID should be set here. Is vms->contents[0] guaranteed to be valid if xen_vm_get_by_name_label succeeds? Okay you check vms->size here before accessing vms->contents[0]. So you should do the same in xenapiDomainSuspend. vms->size check missing here as in xenapiDomainSuspend. Is distro != windows a good indicator for paravirtualization mode? How do you detect the case when you have a non-windows system in HVM mode? OOM check missing. OOM check missing. OOM check missing. Maybe you should unify the way you check for a valid vm set, because you do it both ways around. You forget to check for a valid vm set. value leaks. You call should call virReportOOMError in case of an allcoation error. Check for a valid vm set is missing. Break the for look here an check for OOM error here. mask leaks. Check for a valid vm set is missing. Use virDomainDefPtr. virReportOOMError missing. You cannot copy a raw UUID using strcpy, becaue it isn't a string an can contain 0 values. But strcpy will stop at the first 0 value. Use memcpy(defPtr->uuid, dom->uuid, VIR_UUID_BUFLEN) instead. OOM check missing. OOM check missing. Use xen instead of linux. OOM checks missing. OOM check missing. OOM check missing. OOM check missing. OOM check missing. OOM check missing. Also use STRNEQ instead of !STREQ. Use VIR_DOMAIN_FEATURE_ACPI instead of 0. Use VIR_DOMAIN_FEATURE_APIC instead of 1. Use VIR_DOMAIN_FEATURE_PAE instead of 2. virReportOOMError missing. virReportOOMError missing. Why strdup'ing and freeing bridge? Why not assign it directly? You're leaking defPtr here. Free it using virDomainDefFree. OOM check missing. You're abusing VIR_ERR_NO_DEVICE here. You're abusing VIR_ERR_NO_DEVICE here. You're potentially leaking vms here, because if vms is not NULL but vms->size is then you goto the else branch and vms leaks. I just noticed this pattern here, but this affects probably several functions. You're leaking defPtr here. Free it using virDomainDefFree. You're leaking defPtr here. Free it using virDomainDefFree. Again, missing check for a valid vm set. Missing check for a valid vm set. Missing check for a valid vm set. Don't return a const string here. The result is suppoed to be allocated. You need to strdup it and don't forget the OOM check. Remove ATTRIBUTE_UNUSED from freeMems. > +{ > +    if (maxCells >1 && startCell >0) { > +        xenapiSessionErrorHandler(conn, VIR_ERR_NO_SUPPORT, ""); > +        return -1; > +    } else { > +        freeMems[0] = xenapiNodeGetFreeMemory(conn); > +       return 1; > +    } You should set CURLOPT_SSL_VERIFYHOST to 2, with 1 the common name field is ignored. Or just don't change the SSL config. libcurl defaults to secure settings. > +    CURLcode result = curl_easy_perform(curl); > +    curl_easy_cleanup(curl); > +    return result; Use the virBuffer API instea of snprintf and strcat. Use virAsprintf instead of snprintf and strdup. > +    return ret; OOM check missing. > +    return 0; > +/* Error handling function returns error messages from the server if any */ > +void > +xenapiSessionErrorHandle(virConnectPtr conn, virErrorNumber errNum, > +                         const char *buf, const char *filename, const char *func, size_t lineno) > +{ > +    if (buf==NULL) { > +        char *ret=NULL; > +        ret = returnErrorFromSession(((struct _xenapiPrivate *)(conn->privateData))->session); > +        virReportErrorHelper (conn, VIR_FROM_XENAPI, errNum, filename, func, lineno, _("%s\n"), ret); > +        xen_session_clear_error(((struct _xenapiPrivate *)(conn->privateData))->session); > +        VIR_FREE(ret); > +    } else { > +        virReportErrorHelper (conn, VIR_FROM_XENAPI, errNum, filename, func, lineno, _("%s\n"), buf); > +    } OOM check missing. As mentioned before, you're forgot to copy the UUID here. OOM check missing. OOM check missing. Use the virDomainFeature enum values here too. OOM check missing. OOM check missing. You need to free bridge here in the case mac is NULL but bridge is not. > +        } > +    } > +    return 0; Matthias