Fix coverity warnings (#408)

* Remove warnings with url.ParseRequestURI
* Resolve unchecked error warnings
* Fix null pointer dereference
* Remove dead code
* Remove redundant Boolean return value

Methods client.vcdCheckSupportedVersion, client.checkSupportedVersionConstraint,
and client.apiVersionMatchesConstraint were returning "true" only if
error was nil. Removing the Boolean returning value makes evaluation
more streamlined.

Signed-off-by: Giuseppe Maxia <[email protected]>

.changes/v2.14.0/408-improvements.md

@@ -0,0 +1 @@
+* Remove Coverity warnings from code [GH-408]

govcd/api.go

@@ -88,7 +88,10 @@ func enableDebugShowRequest() {
 //lint:ignore U1000 this function is used on request for debugging purposes
 func disableDebugShowRequest() {
 	debugShowRequestEnabled = false
-	_ = os.Setenv("GOVCD_SHOW_REQ", "")
+	err := os.Setenv("GOVCD_SHOW_REQ", "")
+	if err != nil {
+		util.Logger.Printf("[DEBUG - disableDebugShowRequest] error setting environment variable: %s", err)
+	}
 }
 
 // Enables the debugging hook to show responses as they are processed.
@@ -101,7 +104,10 @@ func enableDebugShowResponse() {
 //lint:ignore U1000 this function is used on request for debugging purposes
 func disableDebugShowResponse() {
 	debugShowResponseEnabled = false
-	_ = os.Setenv("GOVCD_SHOW_RESP", "")
+	err := os.Setenv("GOVCD_SHOW_RESP", "")
+	if err != nil {
+		util.Logger.Printf("[DEBUG - disableDebugShowResponse] error setting environment variable: %s", err)
+	}
 }
 
 // On-the-fly debug hook. If either debugShowRequestEnabled or the environment
@@ -195,15 +201,19 @@ func (client *Client) newRequest(params map[string]string, notEncodedParams map[
 	// If the body contains data - try to read all contents for logging and re-create another
 	// io.Reader with all contents to use it down the line
 	var readBody []byte
+	var err error
 	if body != nil {
-		readBody, _ = ioutil.ReadAll(body)
+		readBody, err = ioutil.ReadAll(body)
+		if err != nil {
+			util.Logger.Printf("[DEBUG - newRequest] error reading body: %s", err)
+		}
 		body = bytes.NewReader(readBody)
 	}
 
-	// Build the request, no point in checking for errors here as we're just
-	// passing a string version of an url.URL struct and http.NewRequest returns
-	// error only if can't process an url.ParseRequestURI().
-	req, _ := http.NewRequest(method, reqUrl.String(), body)
+	req, err := http.NewRequest(method, reqUrl.String(), body)
+	if err != nil {
+		util.Logger.Printf("[DEBUG - newRequest] error getting new request: %s", err)
+	}
 
 	if client.VCDAuthHeader != "" && client.VCDToken != "" {
 		// Add the authorization header
@@ -748,3 +758,16 @@ func (client *Client) RemoveCustomHeader() {
 		client.customHeader = nil
 	}
 }
+
+// ---------------------------------------------------------------------
+// The following functions are needed to avoid strict Coverity warnings
+// ---------------------------------------------------------------------
+
+// urlParseRequestURI returns a URL, discarding the error
+func urlParseRequestURI(href string) *url.URL {
+	apiEndpoint, err := url.ParseRequestURI(href)
+	if err != nil {
+		util.Logger.Printf("[DEBUG - urlParseRequestURI] error parsing request URI: %s", err)
+	}
+	return apiEndpoint
+}

govcd/api_vcd_versions.go

@@ -151,7 +151,10 @@ func (client *Client) vcdFetchSupportedVersions() error {
 func (client *Client) MaxSupportedVersion() (string, error) {
 	versions := make([]*semver.Version, len(client.supportedVersions.VersionInfos))
 	for index, versionInfo := range client.supportedVersions.VersionInfos {
-		version, _ := semver.NewVersion(versionInfo.Version)
+		version, err := semver.NewVersion(versionInfo.Version)
+		if err != nil {
+			return "", fmt.Errorf("error parsing version %s: %s", versionInfo.Version, err)
+		}
 		versions[index] = version
 	}
 	// Sort supported versions in order lowest-highest
@@ -169,24 +172,24 @@ func (client *Client) MaxSupportedVersion() (string, error) {
 
 // vcdCheckSupportedVersion checks if there is at least one specified version exactly matching listed ones.
 // Format example "27.0"
-func (client *Client) vcdCheckSupportedVersion(version string) (bool, error) {
+func (client *Client) vcdCheckSupportedVersion(version string) error {
 	return client.checkSupportedVersionConstraint(fmt.Sprintf("= %s", version))
 }
 
 // Checks if there is at least one specified version matching the list returned by vCD.
 // Constraint format can be in format ">= 27.0, < 32",">= 30" ,"= 27.0".
-func (client *Client) checkSupportedVersionConstraint(versionConstraint string) (bool, error) {
+func (client *Client) checkSupportedVersionConstraint(versionConstraint string) error {
 	for _, versionInfo := range client.supportedVersions.VersionInfos {
 		versionMatch, err := client.apiVersionMatchesConstraint(versionInfo.Version, versionConstraint)
 		if err != nil {
-			return false, fmt.Errorf("cannot match version: %s", err)
+			return fmt.Errorf("cannot match version: %s", err)
 		}
 
 		if versionMatch {
-			return true, nil
+			return nil
 		}
 	}
-	return false, fmt.Errorf("version %s is not supported", versionConstraint)
+	return fmt.Errorf("version %s is not supported", versionConstraint)
 }
 
 func (client *Client) apiVersionMatchesConstraint(version, versionConstraint string) (bool, error) {
@@ -217,7 +220,8 @@ func (client *Client) validateAPIVersion() error {
 	}
 
 	// Check if version is supported
-	if ok, err := client.vcdCheckSupportedVersion(client.APIVersion); !ok || err != nil {
+	err = client.vcdCheckSupportedVersion(client.APIVersion)
+	if err != nil {
 		return fmt.Errorf("API version %s is not supported: %s", client.APIVersion, err)
 	}
 

govcd/catalog.go

@@ -218,8 +218,17 @@ func (cat *Catalog) UploadOvf(ovaFileName, itemName, description string, uploadP
 
 	uploadError := *new(error)
 
-	//sending upload process to background, this allows no to lock and return task to client
-	go uploadFiles(cat.client, vappTemplate, &ovfFileDesc, tmpDir, filesAbsPaths, uploadPieceSize, progressCallBack, &uploadError, isOvf)
+	// sending upload process to background, this allows not to lock and return task to client
+	// The error should be captured in uploadError, but just in case, we add a logging for the
+	// main error
+	go func() {
+		err = uploadFiles(cat.client, vappTemplate, &ovfFileDesc, tmpDir, filesAbsPaths, uploadPieceSize, progressCallBack, &uploadError, isOvf)
+		if err != nil {
+			util.Logger.Println(strings.Repeat("*", 80))
+			util.Logger.Printf("*** [DEBUG - UploadOvf] error calling uploadFiles: %s\n", err)
+			util.Logger.Println(strings.Repeat("*", 80))
+		}
+	}()
 
 	var task Task
 	for _, item := range vappTemplate.Tasks.Task {
@@ -314,6 +323,7 @@ func uploadFiles(client *Client, vappTemplate *types.VAppTemplate, ovfFileDesc *
 			return err
 		}
 	}
+	uploadError = nil
 	return nil
 }
 

govcd/disk.go

@@ -214,11 +214,10 @@ func (disk *Disk) Delete() (Task, error) {
 
 // Refresh the disk information by disk href
 func (disk *Disk) Refresh() error {
-	util.Logger.Printf("[TRACE] Disk refresh, HREF: %s\n", disk.Disk.HREF)
-
 	if disk.Disk == nil || disk.Disk.HREF == "" {
 		return fmt.Errorf("cannot refresh, Object is empty")
 	}
+	util.Logger.Printf("[TRACE] Disk refresh, HREF: %s\n", disk.Disk.HREF)
 
 	unmarshalledDisk := &types.Disk{}
 

govcd/edgegateway.go

@@ -120,7 +120,7 @@ func (egw *EdgeGateway) AddDhcpPool(network *types.OrgVDCNetwork, dhcppool []int
 	for {
 		buffer := bytes.NewBufferString(xml.Header + string(output))
 
-		apiEndpoint, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+		apiEndpoint := urlParseRequestURI(egw.EdgeGateway.HREF)
 		apiEndpoint.Path += "/action/configureServices"
 
 		req := egw.client.NewRequest(map[string]string{}, http.MethodPost, *apiEndpoint, buffer)
@@ -197,7 +197,7 @@ func (egw *EdgeGateway) RemoveNATPortMapping(natType, externalIP, externalPort,
 		NatService: newNatService,
 	}
 
-	apiEndpoint, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	apiEndpoint := urlParseRequestURI(egw.EdgeGateway.HREF)
 	apiEndpoint.Path += "/action/configureServices"
 
 	// Return the task
@@ -263,7 +263,7 @@ func (egw *EdgeGateway) RemoveNATRuleAsync(id string) (Task, error) {
 		NatService: natServiceToUpdate,
 	}
 
-	egwConfigureHref, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	egwConfigureHref := urlParseRequestURI(egw.EdgeGateway.HREF)
 	egwConfigureHref.Path += "/action/configureServices"
 
 	// Return the task
@@ -428,7 +428,7 @@ func (egw *EdgeGateway) UpdateNatRuleAsync(natRule *types.NatRule) (Task, error)
 		NatService: natServiceToUpdate,
 	}
 
-	egwConfigureHref, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	egwConfigureHref := urlParseRequestURI(egw.EdgeGateway.HREF)
 	egwConfigureHref.Path += "/action/configureServices"
 
 	// Return the task
@@ -506,7 +506,7 @@ func (egw *EdgeGateway) AddNATRuleAsync(ruleDetails NatRule) (Task, error) {
 		NatService: newNatService,
 	}
 
-	egwConfigureHref, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	egwConfigureHref := urlParseRequestURI(egw.EdgeGateway.HREF)
 	egwConfigureHref.Path += "/action/configureServices"
 
 	// Return the task
@@ -654,7 +654,7 @@ func (egw *EdgeGateway) AddNATPortMappingWithUplink(network *types.OrgVDCNetwork
 		NatService: newNatService,
 	}
 
-	apiEndpoint, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	apiEndpoint := urlParseRequestURI(egw.EdgeGateway.HREF)
 	apiEndpoint.Path += "/action/configureServices"
 
 	// Return the task
@@ -687,7 +687,7 @@ func (egw *EdgeGateway) CreateFirewallRules(defaultAction string, rules []*types
 	for {
 		buffer := bytes.NewBufferString(xml.Header + string(output))
 
-		apiEndpoint, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+		apiEndpoint := urlParseRequestURI(egw.EdgeGateway.HREF)
 		apiEndpoint.Path += "/action/configureServices"
 
 		req := egw.client.NewRequest(map[string]string{}, http.MethodPost, *apiEndpoint, buffer)
@@ -837,7 +837,7 @@ func (egw *EdgeGateway) Remove1to1Mapping(internal, external string) (Task, erro
 	// Fix
 	newEdgeConfig.NatService.IsEnabled = true
 
-	apiEndpoint, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	apiEndpoint := urlParseRequestURI(egw.EdgeGateway.HREF)
 	apiEndpoint.Path += "/action/configureServices"
 
 	// Return the task
@@ -932,7 +932,7 @@ func (egw *EdgeGateway) Create1to1Mapping(internal, external, description string
 
 	newEdgeConfig.FirewallService.FirewallRule = append(newEdgeConfig.FirewallService.FirewallRule, fwout)
 
-	apiEndpoint, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	apiEndpoint := urlParseRequestURI(egw.EdgeGateway.HREF)
 	apiEndpoint.Path += "/action/configureServices"
 
 	// Return the task
@@ -950,7 +950,7 @@ func (egw *EdgeGateway) AddIpsecVPN(ipsecVPNConfig *types.EdgeGatewayServiceConf
 
 	ipsecVPNConfig.Xmlns = types.XMLNamespaceVCloud
 
-	apiEndpoint, _ := url.ParseRequestURI(egw.EdgeGateway.HREF)
+	apiEndpoint := urlParseRequestURI(egw.EdgeGateway.HREF)
 	apiEndpoint.Path += "/action/configureServices"
 
 	// Return the task

govcd/filter_engine.go

@@ -291,10 +291,6 @@ func searchByFilter(queryByMetadata queryByMetadataFunc, queryWithMetadataFields
 			return nil, explanation, fmt.Errorf("search for oldest item failed. Empty dates found for items %v", emptyDatesFound)
 		}
 	}
-	if searchEarliest || searchLatest {
-		// We should never reach this point, as a failure for newest or oldest item was caught above, but just in case
-		return nil, explanation, fmt.Errorf("search for oldest or earliest item failed. No reason found")
-	}
 	return candidatesByConditions, explanation, nil
 }
 

govcd/filter_helpers.go

@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/vmware/go-vcloud-director/v2/types/v56"
+	"github.com/vmware/go-vcloud-director/v2/util"
 )
 
 // This file contains functions that help create tests for filtering.
@@ -152,18 +153,33 @@ func makeDateFilter(items []DateItem) ([]FilterMatch, error) {
 			earliestFound = true
 		}
 		exactFilter := NewFilterDef()
-		_ = exactFilter.AddFilter(types.FilterDate, "=="+item.Date)
+		err = exactFilter.AddFilter(types.FilterDate, "=="+item.Date)
+		if err != nil {
+			return nil, fmt.Errorf("error adding filter '%s' '%s': %s", types.FilterDate, "=="+item.Date, err)
+		}
 		filters = append(filters, FilterMatch{exactFilter, item.Name, item.Entity, item.EntityType})
 	}
 
 	if earliestFound && latestFound && earliestDate != latestDate {
 		earlyFilter := NewFilterDef()
-		_ = earlyFilter.AddFilter(types.FilterDate, "<"+latestDate)
-		_ = earlyFilter.AddFilter(types.FilterEarliest, "true")
+		err := earlyFilter.AddFilter(types.FilterDate, "<"+latestDate)
+		if err != nil {
+			return nil, err
+		}
+		err = earlyFilter.AddFilter(types.FilterEarliest, "true")
+		if err != nil {
+			return nil, err
+		}
 
 		lateFilter := NewFilterDef()
-		_ = lateFilter.AddFilter(types.FilterDate, ">"+earliestDate)
-		_ = lateFilter.AddFilter(types.FilterLatest, "true")
+		err = lateFilter.AddFilter(types.FilterDate, ">"+earliestDate)
+		if err != nil {
+			return nil, err
+		}
+		err = lateFilter.AddFilter(types.FilterLatest, "true")
+		if err != nil {
+			return nil, err
+		}
 
 		filters = append(filters, FilterMatch{earlyFilter, earliestName, earliestEntity, entityType})
 		filters = append(filters, FilterMatch{lateFilter, latestName, latestEntity, entityType})
@@ -454,15 +470,25 @@ func ipToRegex(ip string) string {
 // strToRegex creates a regular expression that matches perfectly with the input query
 func strToRegex(s string) string {
 	var result strings.Builder
-	result.WriteString("^")
+	var err error
+	_, err = result.WriteString("^")
+	if err != nil {
+		util.Logger.Printf("[DEBUG - strToRegex] error writing to string: %s", err)
+	}
 	for _, ch := range s {
 		if ch == '.' {
-			result.WriteString(fmt.Sprintf("\\%c", ch))
+			_, err = result.WriteString(fmt.Sprintf("\\%c", ch))
 		} else {
-			result.WriteString(fmt.Sprintf("[%c]", ch))
+			_, err = result.WriteString(fmt.Sprintf("[%c]", ch))
+		}
+		if err != nil {
+			util.Logger.Printf("[DEBUG - strToRegex] error writing to string: %s", err)
 		}
 	}
-	result.WriteString("$")
+	_, err = result.WriteString("$")
+	if err != nil {
+		util.Logger.Printf("[DEBUG - strToRegex] error writing to string: %s", err)
+	}
 	return result.String()
 }
 

govcd/media.go

@@ -132,7 +132,17 @@ func executeUpload(client *Client, media *types.Media, mediaFilePath, mediaName
 		uploadError:              &uploadError,
 	}
 
-	go uploadFile(client, mediaFilePath, details)
+	// sending upload process to background, this allows not to lock and return task to client
+	// The error should be captured in details.uploadError, but just in case, we add a logging for the
+	// main error
+	go func() {
+		_, err = uploadFile(client, mediaFilePath, details)
+		if err != nil {
+			util.Logger.Println(strings.Repeat("*", 80))
+			util.Logger.Printf("*** [DEBUG - executeUpload] error calling uploadFile: %s\n", err)
+			util.Logger.Println(strings.Repeat("*", 80))
+		}
+	}()
 
 	var task Task
 	for _, item := range media.Tasks.Task {

govcd/metadata.go

@@ -7,7 +7,6 @@ package govcd
 import (
 	"fmt"
 	"net/http"
-	"net/url"
 	"strings"
 
 	"github.com/vmware/go-vcloud-director/v2/types/v56"
@@ -116,7 +115,7 @@ func (vapp *VApp) DeleteMetadata(key string) (Task, error) {
 // Deletes metadata (type MetadataStringValue) from the vApp
 // TODO: Support all MetadataTypedValue types with this function
 func deleteMetadata(client *Client, key string, requestUri string) (Task, error) {
-	apiEndpoint, _ := url.ParseRequestURI(requestUri)
+	apiEndpoint := urlParseRequestURI(requestUri)
 	apiEndpoint.Path += "/metadata/" + key
 
 	// Return the task
@@ -142,7 +141,7 @@ func addMetadata(client *Client, key string, value string, requestUri string) (T
 		},
 	}
 
-	apiEndpoint, _ := url.ParseRequestURI(requestUri)
+	apiEndpoint := urlParseRequestURI(requestUri)
 	apiEndpoint.Path += "/metadata/" + key
 
 	// Return the task