Re: [PATCH 1/3] Set net_<interface>_client{id, uuid} variables from DHCP options

[Javier Martinez Canillas]

Hello Daniel,

On 10/15/19 4:19 PM, Daniel Kiper wrote:
> On Sat, Oct 05, 2019 at 12:44:25AM +0200, Javier Martinez Canillas wrote:
>> From: Paulo Flabiano Smorigo <address@hidden>
>>
>> This patch sets a net_<interface>_clientid and net_<interface>_clientuuid
>> GRUB environment variables, using the DHCP client ID and UUID options if
>> these are found.
>>
>> In the same way than net_<interface>_<option> variables are set for other
>> options such domain name, boot file, next server, etc.
>>
>> Signed-off-by: Paulo Flabiano Smorigo <address@hidden>
>> Signed-off-by: Javier Martinez Canillas <address@hidden>
> 
> In general "Reviewed-by: Daniel Kiper <address@hidden>"
> but a few nit picks below...
>

Thanks for the review.

>> ---
>>
>>  grub-core/net/bootp.c | 85 +++++++++++++++++++++++++++++++++++++++----
>>  include/grub/net.h    |  2 +
>>  2 files changed, 79 insertions(+), 8 deletions(-)
>>
>> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
>> index 04cfbb04504..0e6e41a1699 100644
>> --- a/grub-core/net/bootp.c
>> +++ b/grub-core/net/bootp.c
>> @@ -95,6 +95,49 @@ enum
>>  /* Max timeout when waiting for BOOTP/DHCP reply */
>>  #define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
>>
>> +static char *
>> +grub_env_write_readonly (struct grub_env_var *var __attribute__ ((unused)),
>> +                         const char *val __attribute__ ((unused)))
> 
> s/grub_env_write_readonly/grub_env_ro_write/?
>

I noticed that this function already exists in grub-core/net/net.c, take a
look to commit e4dbf247b65 ("add grub_env_set_net_property function").

So I'll just remove this and the cryptic set_env_limn_ro() function.

>> +{
>> +  return NULL;
>> +}
>> +
>> +static void
>> +set_env_limn_ro (const char *intername, const char *suffix,
>> +                 const char *value, grub_size_t len)
> 
> I have hard time reading this. What does "limn" stand for?
> Could we make the name of this function less cryptic?
>

Yes, same. But this function also already exists in grub-core/net/net.c as
mentioned. It's called grub_env_set_net_property() and I'll use that.

Sorry for missing that this patch could be much more simpler after a rebase
to the latest GRUB version.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat