Re: [PATCH v2 2/2] arm/efi: Use dom0less configuration when using EFI boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2021 16:13, Luca Fancellu wrote:
> +static unsigned int __init allocate_dom0less_file(EFI_FILE_HANDLE dir_handle,
> +                                                  const char *name,
> +                                                  unsigned int name_len)
> +{
> +    dom0less_module_name* file_name;
> +    union string module_name;
> +    unsigned int ret_idx;
> +
> +    /*
> +     * Check if there is any space left for a domU module, the variable
> +     * dom0less_modules_available is updated each time we use read_file(...)
> +     * successfully.
> +     */
> +    if ( !dom0less_modules_available )
> +        blexit(L"No space left for domU modules");
> +
> +    module_name.s = (char*) name;

Unfortunately there are too many style issues in these Arm additions to
really enumerate; I'd like to ask that you go through yourself with
./CODING_STYLE, surrounding code, and review comments on earlier patches
of yours in mind. This cast stands out, though: I'm pretty sure you were
told before that casts are often dangerous and hence should be avoided
whenever (easily) possible. There was a prior case where union string
was used in a similar way, not all that long ago. Hence why it now has
a "const char *" member. (That's still somewhat risky, but imo way
better than a cast.)

> @@ -1361,12 +1360,21 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>          cfg.addr = 0;
>  
> -        dir_handle->Close(dir_handle);
> -
>          if ( gop && !base_video )
>              gop_mode = efi_find_gop_mode(gop, cols, rows, depth);
>      }
>  
> +    /*
> +     * Check if a proper configuration is provided to start Xen:
> +     *  - Dom0 specified (minimum required)
> +     *  - Dom0 and DomU(s) specified
> +     *  - DomU(s) specified
> +     */
> +    if ( !efi_arch_check_dom0less_boot(dir_handle) && !kernel.addr )
> +        blexit(L"No Dom0 kernel image specified.");
> +
> +    dir_handle->Close(dir_handle);

So far I was under the impression that handles and alike need closing
before calling Exit(), to prevent resource leaks. While I will admit
that likely there are more (pre-existing) affected paths, I think that
- if that understanding of mine is correct - it would be nice to avoid
adding yet more instances.

Jan