On 05/03/2013 08:53 AM, Michal Privoznik wrote:
> ---
> src/esx/esx_driver.c | 43 ++++++--------------
> src/esx/esx_interface_driver.c | 7 +---
> src/esx/esx_network_driver.c | 28 ++++---------
> src/esx/esx_storage_backend_iscsi.c | 21 +++-------
> src/esx/esx_storage_backend_vmfs.c | 16 ++------
> src/esx/esx_util.c | 45 +++++----------------
> src/esx/esx_vi.c | 79 +++++++------------------------------
> src/esx/esx_vi_types.c | 36 +++--------------
> 8 files changed, 58 insertions(+), 217 deletions(-)
>
> @@ -751,14 +747,9 @@ esxConnectToHost(esxPrivate *priv,
> VIR_WARN("The server is in maintenance mode");
> }
>
> - if (*vCenterIpAddress != NULL) {
> - *vCenterIpAddress = strdup(*vCenterIpAddress);
Old code: if string is not NULL, replace it with a copy from the heap...
> -
> - if (*vCenterIpAddress == NULL) {
> - virReportOOMError();
> - goto cleanup;
> - }
> - }
> + if (!*vCenterIpAddress &&
> + VIR_STRDUP(*vCenterIpAddress, *vCenterIpAddress) < 0)
New code: if string is NULL, copy NULL to the heap. Oops. Drop the !
> @@ -801,9 +792,7 @@ esxConnectToVCenter(esxPrivate *priv,
> }
>
> if (conn->uri->user != NULL) {
> - username = strdup(conn->uri->user);
> -
> - if (username == NULL) {
> + if (VIR_STRDUP(username, conn->uri->user) < 0) {
> virReportOOMError();
Drop the oom error here.
> @@ -1278,12 +1267,8 @@ esxConnectGetHostname(virConnectPtr conn)
> }
>
> if (domainName == NULL || strlen(domainName) < 1) {
Pre-existing, but I'd have written the second condition as
'!*domainName' (which is O(1) regardless of the length of domainName,
whereas strlen()<1 is O(n) if the compiler doesn't optimize well)
> @@ -1294,7 +1279,7 @@ esxConnectGetHostname(virConnectPtr conn)
> cleanup:
> /*
> * If we goto cleanup in case of an error then complete is still NULL,
> - * either strdup returned NULL or virAsprintf failed. When virAsprintf
> + * either VIR_STRDUP returned NULL or virAsprintf failed. When virAsprintf
Technically VIR_STRDUP returns -1, not NULL, if you want to touch up
this comment.
> +++ b/src/esx/esx_util.c
> @@ -171,23 +159,12 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri)
> }
> }
>
> - if (uri->path != NULL) {
> - (*parsedUri)->path = strdup(uri->path);
> -
> - if ((*parsedUri)->path == NULL) {
> - virReportOOMError();
> - goto cleanup;
> - }
> - }
> -
> - if ((*parsedUri)->transport == NULL) {
> - (*parsedUri)->transport = strdup("https");
> + if (uri->path && VIR_STRDUP((*parsedUri)->path, uri->path) < 0)
> + goto cleanup;
Another argument in favor of allowing a NULL source argument.
> +++ b/src/esx/esx_vi_types.c
> @@ -1327,16 +1310,7 @@ esxVI_String_DeserializeValue(xmlNodePtr node, char **value)
>
> *value = (char *)xmlNodeListGetString(node->doc, node->children, 1);
>
> - if (*value == NULL) {
> - *value = strdup("");
Old code: if xmlNodeListGetString found nothing, set *value to an empty
string...
> -
> - if (*value == NULL) {
> - virReportOOMError();
> - return -1;
> - }
> - }
> -
> - return 0;
> + return value ? 0 : VIR_STRDUP(*value, "");
New code: since 'value' is non-NULL, unconditionally return 0 even if
*value is still NULL. Oops. Missing a *
ACK if you fix the bugs I called out.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list