Re: [PATCH v2 16/23] OvmfPkg/VirtioRngDxe: map host address to device address

[Brijesh Singh <brijesh.singh@amd.com>]

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;