Re: [Xen-devel] [PATCH V5 10/15] Add arch specific module handling to read_file()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.14 at 00:50, <roy.franz@xxxxxxxxxx> wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -56,12 +56,13 @@ static void noreturn blexit(const CHAR16 *str);
>  static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
>  static char *get_value(const struct file *cfg, const char *section,
>                                const char *item);
> -static void  split_value(char *s);
> +static char *split_string(char *s);
>  static CHAR16 *s2w(union string *str);
>  static char *w2s(const union string *str);
> -static bool_t  read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> -                               struct file *file);
> +static size_t wstrlen(const CHAR16 * s);
>  static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
> +static bool_t read_file(EFI_FILE_HANDLE dir_handle, struct file *file,
> +                               CHAR16 *filename, char *options);

Please don't needlessly move this declaration down.

> @@ -115,6 +116,15 @@ static void __init DisplayUint(UINT64 Val, INTN Width)
>      PrintStr(PrintString);
>  }
>  
> +static size_t __init wstrlen(const CHAR16 * s)
> +{
> +     const CHAR16 *sc;
> +
> +     for (sc = s; *sc != L'\0'; ++sc)
> +             /* nothing */;
> +     return sc - s;
> +}

Coding style (many issues).

> @@ -404,18 +414,33 @@ static CHAR16 *__init point_tail(CHAR16 *fn)
>              break;
>          }
>  }
> +/*
> + * Truncate string at first space, and return pointer
> + * to remainder of string.

... (if any).

> + */
> +static char * __init split_string(char *s)
> +{
> +    while ( *s && !isspace(*s) )
> +        ++s;
> +    if ( *s )
> +    {
> +        *s = 0;
> +        return(s + 1);

No parentheses here please.

> +    }
> +    return NULL;
> +}
>  
> -static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> -                               struct file *file)
> +static bool_t __init read_file(EFI_FILE_HANDLE dir_handle, struct file *file,
> +                               CHAR16 *filename, char *options)

Is the renaming from "name" to "filename" really necessary/useful?
And the flipping of the order of pre-existing parameters?

> @@ -659,18 +678,19 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>      EFI_LOADED_IMAGE *loaded_image;
>      EFI_STATUS status;
>      unsigned int i, argc;
> -    CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
> +    CHAR16 **argv, *options = NULL;
>      UINTN cols, rows, depth, size, info_size, gop_mode = ~0;
>      EFI_HANDLE *handles = NULL;
>      EFI_SHIM_LOCK_PROTOCOL *shim_lock;
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>      EFI_FILE_HANDLE dir_handle;
> -    union string section = { NULL }, name;
> +    union string section = { NULL }, name, file_name, cfg_file_name = {NULL};

Your addition should be consistent with existing code (blanks around
NULL). But - why are you changing the types of the two variables in
the first place? Afaics all references to them now are using the .w
member access, suggesting that this is just a remnant of your earlier
version changes.

> @@ -274,8 +275,8 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE 
> dir_handle, char *sect
>      if ( name.s )
>      {
>          microcode_set_module(mbi.mods_count);
> -        split_value(name.s);
> -        read_file(dir_handle, s2w(&name), &ucode);
> +        options = split_string(name.s);
> +        read_file(dir_handle, &ucode, s2w(&name), options);

Perhaps rather NULL instead of options?

> @@ -575,3 +576,30 @@ static void __init efi_arch_memory(void)
>      l3_bootmap[l3_table_offset(xen_phys_start + (8 << L2_PAGETABLE_SHIFT) - 
> 1)] =
>          l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR);
>  }
> +
> +static void __init efi_arch_handle_module(struct file *file, const CHAR16 
> *name,
> +                                          char *options)
> +{
> +    union string local_name;
> +    void *ptr;
> +
> +    /*
> +     * Make a copy, as conversion is destructive, and caller still wants
> +     * wide string available after this call returns.
> +     */
> +    if ( efi_bs->AllocatePool(EfiLoaderData, (wstrlen(name) + 1) * 
> sizeof(*name),
> +                              &ptr) != EFI_SUCCESS )
> +        blexit(L"ERROR Unable to allocate string buffer\r\n");

Iirc I said this before, but just in case: No explicit newline on strings
passed to blexit() please.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel