From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EAAC421AEB0A1 for ; Tue, 22 Aug 2017 09:25:37 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4DC1B5277A; Tue, 22 Aug 2017 16:28:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4DC1B5277A Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-92.phx2.redhat.com [10.3.116.92]) by smtp.corp.redhat.com (Postfix) with ESMTP id C23857840E; Tue, 22 Aug 2017 16:28:03 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502710605-8058-1-git-send-email-brijesh.singh@amd.com> <1502710605-8058-17-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Tue, 22 Aug 2017 18:28:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 22 Aug 2017 16:28:10 +0000 (UTC) Subject: Re: [PATCH v2 16/23] OvmfPkg/VirtioRngDxe: map host address to device address X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Aug 2017 16:25:38 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/22/17 17:44, Brijesh Singh wrote: > > > On 08/21/2017 09:05 AM, Laszlo Ersek wrote: > > [...] > >> >>> for (Index = 0; Index < RNGValueLength; Index++) { >>> RNGValue[Index] = Buffer[Index]; >>> } >>> Status = EFI_SUCCESS; >>> + // >>> + // Buffer is already Unmaped(), goto free it. >>> + // >>> + goto FreeBuffer; >> >> (6) We restrict "goto" statements to error handling: >> >> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html >> >> >>> 5.7.3.8 Goto Statements should not be used (in general) >>> [...] >>> It is common to use goto for error handling and thus, exiting a >>> routine in an error case is the only legal use of a goto. [...] >> >> So please open-code the FreePool() call and the "return" statement >> here. >> > > I was wondering if something like this is acceptable ? Actually since > we need to handle the Unmap due to error from VirtioFlush hence I was > trying to minimize the adding new state variables or additional checks > inside the for loop. > > @@ -178,17 +194,34 @@ VirtioRngGetRNG ( > if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != > EFI_SUCCESS) { > Status = EFI_DEVICE_ERROR; > - goto FreeBuffer; > + goto UnmapBuffer; > } > ASSERT (Len > 0); > ASSERT (Len <= BufferSize); > } > > + // > + // Unmap the device buffer before accessing it. > + // > + Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + if (EFI_ERROR (Status)) { > + goto FreeBuffer; > + } > + > for (Index = 0; Index < RNGValueLength; Index++) { > RNGValue[Index] = Buffer[Index]; > } > Status = EFI_SUCCESS; > > +UnmapBuffer: > + // > + // If we are reached here due to the error then unmap the buffer > otherwise > + // the buffer is already unmapped after VirtioFlush(). > + // > + if (EFI_ERROR (Status)) { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + } > + > FreeBuffer: > FreePool ((VOID *)Buffer); > return Status; > This is perfectly fine, as long as it doesn't get overly complex. It keeps goto's limited to cascading error handling, only part of the error handling becomes optional if the ownership of Mapping has been transferred before. In functions where you have a simple mapping, this doesn't look difficult, but more care might be required for functions where you have several mappings (some of which could be optional, dependent on input parameters or other conditions). IMO, the pattern should work nonetheless: > if (InputCondition1) { > Status = Map (&Mapping1); > if (EFI_ERROR (Status)) { > goto FreeBuffer; > } > } > > if (InputCondition2) { > Status = Map (&Mapping2); > if (EFI_ERROR (Status)) { > goto Unmap1; > } > } > > if (InputCondition3) { > Status = Map (&Mapping3); > if (EFI_ERROR (Status)) { > goto Unmap2; > } > } > > Status = DoSomethingElse (); > if (EFI_ERROR (Status)) { > goto Unmap3; > } > > // > // Now unmap stuff for processing results. > // > > if (InputCondition3) { > Status = Unmap (Mapping3); > if (EFI_ERROR (Status)) { > goto Unmap2; > } > } > > if (InputCondition2) { > Status = Unmap (Mapping2); > if (EFI_ERROR (Status)) { > goto Unmap1; > } > } > > if (InputCondition1) { > Status = Unmap (Mapping1); > if (EFI_ERROR (Status)) { > goto FreeBuffer; > } > } > > // > // Process results. > // > ... > > Status = EFI_SUCCESS; > > Unmap3: > if (EFI_ERROR (Status) && InputCondition3) { > Unmap (Mapping3); > } > > Unmap2: > if (EFI_ERROR (Status) && InputCondition2) { > Unmap (Mapping2); > } > > Unmap1: > if (EFI_ERROR (Status) && InputCondition1) { > Unmap (Mapping1); > } > > FreeBuffer: > // > // some rollback action that has to be performed unconditionally on > // exit > // > ... > > return Status; This needs no additional state variables. The depth of the successful map/unmap sequence is captured with the goto statements and the labels. And, on the exit path, unmapping of each mapping depends on two conditions: the exit path being taken due to success or error, and on the given mapping being relevant in the first place. Thanks, Laszlo