|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V5 10/15] Add arch specific module handling to read_file()
On Tue, Sep 23, 2014 at 6:23 PM, Roy Franz <roy.franz@xxxxxxxxxx> wrote:
> On Mon, Sep 22, 2014 at 6:57 PM, Roy Franz <roy.franz@xxxxxxxxxx> wrote:
>> On Mon, Sep 22, 2014 at 5:44 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> 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.
>>>
>> I'll fix this.
>>
>>>> @@ -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).
>>
>> static size_t __init wstrlen(const CHAR16 *s)
>> {
>> const CHAR16 *sc;
>>
>> for ( sc = s; *sc != L'\0'; ++sc )
>> ;
>> return sc - s;
>> }
>>
>> Is the above OK? Not sure what else to change here...
>>
>>>
>>>> @@ -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).
>>>
>>
>> Sure.
>>
>>
>>>> + */
>>>> +static char * __init split_string(char *s)
>>>> +{
>>>> + while ( *s && !isspace(*s) )
>>>> + ++s;
>>>> + if ( *s )
>>>> + {
>>>> + *s = 0;
>>>> + return(s + 1);
>>>
>>> No parentheses here please.
>>
>> sure
>>
>>
>>>
>>>> + }
>>>> + 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?
>>
>> I'll fix this.
>>
>>>
>>>> @@ -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.
>>>
>>
>> Yes, this is leftover type changes from a previous version. Both
>> these should be able to to be
>> reverted to simple CHAR16 *
>>>> @@ -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?
>>
>> Yup.
>>
> OK, I looked at this a little more, and the original code would take
> anything after the ucode filename
> and put it into the mbi.string field for the ucode module. All the
> modules where treated the same in this
> regard - they could all have options.
>
> If ucode never has options, I can remove this, but this would be a
> change in behavior.
The EFI config file documentation specifies just the filename, so I
have simplified this
in the next version.
>
>
>>>
>>>> @@ -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.
>> Yes.
>>>
>>> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |