Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function

[Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>]

On 12/12/2016 01:27 PM, Andrei Borzenkov wrote:
> While I agree with your reasons, it belongs to separate commit, and
> definitely is out of place if you say in commit message "I'm moving
> piece of code". Actually, it is not related to this patch series, so
> feel free to send this cleanup as separate patch.

Thank you. I'll stick to 'return 0' for this series.

> 
> On Mon, Dec 12, 2016 at 12:47 PM, Stanislav Kholmanskikh
> <stanislav.kholmanskikh@oracle.com> wrote:
>>
>>
>> On 12/05/2016 06:52 PM, Daniel Kiper wrote:
>>> On Fri, Dec 02, 2016 at 06:10:03PM +0300, Stanislav Kholmanskikh wrote:
>>>> In the current code search_net_devices() uses the "alloc-mem" command
>>>> from the IEEE1275 User Interface for allocation of the transmit buffer
>>>> for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
>>>>
>>>> I don't have hardware where this flag is set to verify if this
>>>> workaround is still needed. However, further changes to ofnet will
>>>> require to execute this workaround one more time. Therefore, to
>>>> avoid possible duplication of code I'm moving this piece of
>>>> code into a function.
>>>>
>>>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>>>> ---
>>>>  grub-core/net/drivers/ieee1275/ofnet.c |   71 ++++++++++++++++++++------------
>>>>  1 files changed, 44 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
>>>> index 8332d34..25559c8 100644
>>>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>>>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>>>> @@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
>>>>    }
>>>>  }
>>>>
>>>> +/* Allocate memory with alloc-mem */
>>>> +static void *
>>>> +grub_ieee1275_alloc_mem (grub_size_t len)
>>>> +{
>>>> +  struct alloc_args
>>>> +  {
>>>> +    struct grub_ieee1275_common_hdr common;
>>>> +    grub_ieee1275_cell_t method;
>>>> +    grub_ieee1275_cell_t len;
>>>> +    grub_ieee1275_cell_t catch;
>>>> +    grub_ieee1275_cell_t result;
>>>> +  }
>>>> +  args;
>>>> +
>>>> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
>>>> +    {
>>>> +      grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
>>>> +      return NULL;
>>>> +    }
>>>> +
>>>> +  INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
>>>> +  args.len = len;
>>>> +  args.method = (grub_ieee1275_cell_t) "alloc-mem";
>>>> +
>>>> +  if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
>>>> +    {
>>>> +      grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
>>>> +      return NULL;
>>>> +    }
>>>> +  else
>>>> +    return (void *)args.result;
>>>> +}
>>>> +
>>>> +static void *
>>>> +ofnet_alloc_netbuf (grub_size_t len)
>>>> +{
>>>> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
>>>> +    return grub_ieee1275_alloc_mem (len);
>>>> +  else
>>>> +    return grub_zalloc (len);
>>>> +}
>>>> +
>>>>  static int
>>>>  search_net_devices (struct grub_ieee1275_devalias *alias)
>>>>  {
>>>> @@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>>>>
>>>>    card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
>>>>
>>>> -  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
>>>> -    {
>>>> -      struct alloc_args
>>>> -      {
>>>> -    struct grub_ieee1275_common_hdr common;
>>>> -    grub_ieee1275_cell_t method;
>>>> -    grub_ieee1275_cell_t len;
>>>> -    grub_ieee1275_cell_t catch;
>>>> -    grub_ieee1275_cell_t result;
>>>> -      }
>>>> -      args;
>>>> -      INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
>>>> -      args.len = card->txbufsize;
>>>> -      args.method = (grub_ieee1275_cell_t) "alloc-mem";
>>>> -
>>>> -      if (IEEE1275_CALL_ENTRY_FN (&args) == -1
>>>> -      || args.catch)
>>>> -    {
>>>> -      card->txbuf = 0;
>>>> -      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
>>>> -    }
>>>> -      else
>>>> -    card->txbuf = (void *) args.result;
>>>> -    }
>>>> -  else
>>>> -    card->txbuf = grub_zalloc (card->txbufsize);
>>>> +  card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
>>>>    if (!card->txbuf)
>>>>      {
>>>>        grub_free (ofdata->path);
>>>>        grub_free (ofdata);
>>>>        grub_free (card);
>>>>        grub_print_error ();
>>>> -      return 0;
>>>> +      return 1;
>>>
>>> Hmmm... This looks like an error...
>>
>> Look, here is what I see.
>>
>> The search_net_devices() function is called from grub_ofnet_findcards() as:
>>
>> grub_ieee1275_devices_iterate (search_net_devices);
>>
>> The grub_ieee1275_devices_iterate(hook) function is defined in
>> grub-core/kern/ieee1275/openfw.c and what it does is basically calling
>> the hook for each IEEE1275 device in the tree until:
>> a) there are no more devices
>> b) the hook returns a value != 1
>>
>> So if search_net_devices() returns 1 it means that further probing for
>> network cards is stopped.
>>
>> I think that stopping further probes when a memory allocation function
>> fails is fine and it aligns with the existing code at the top of the
>> function, i.e. handling of the cases when allocating memory for 'card'
>> and 'ofdata' fails.
>>
>> If I'm not mistaken, we may also need to update the block:
>>
>>   if (need_suffix)
>>     ofdata->path = grub_malloc (grub_strlen (alias->path) + sizeof
>> (SUFFIX));
>>   else
>>     ofdata->path = grub_malloc (grub_strlen (alias->path) + 1);
>>   if (!ofdata->path)
>>     {
>>       grub_print_error ();
>>       return 0;
>>     }
>>
>> and add a 'return 1' + free some memory there.
>>
>> As for the other block:
>>
>>   pprop = (grub_uint8_t *) &prop;
>>   if (grub_ieee1275_get_property (devhandle, "mac-address",
>>                                   pprop, sizeof(prop), &prop_size)
>>       && grub_ieee1275_get_property (devhandle, "local-mac-address",
>>                                      pprop, sizeof(prop), &prop_size))
>>     {
>>       grub_error (GRUB_ERR_IO, "Couldn't retrieve mac address.");
>>       grub_print_error ();
>>       return 0;
>>     }
>>
>> it seems we need to add free memory procedures here as well, but I'm not
>> sure we need to return 1 here.
>>
>>
>>
>>>
>>> Daniel
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>