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

[Laszlo Ersek <lersek@redhat.com>]

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