Re: [PATCH, RFT] ubootnet

[Francesco Lavra]

On 04/10/2013 01:19 AM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On Raspberry pi u-boot doesn't support network. Could someone test this?

I'm unable to test it, I just have a couple of minor comments.

> --- grub-core/net/drivers/uboot/ubootnet.c    1970-01-01 00:00:00 +0000
> +++ grub-core/net/drivers/uboot/ubootnet.c    2013-04-09 22:28:48 +0000
[...]
> +static struct grub_net_buff *
> +get_card_packet (struct grub_net_card *dev)
> +{
> +  int rc;
> +  struct ubootnet_data *data = dev->data;
> +  grub_uint64_t start_time;
> +  struct grub_net_buff *nb;
> +  int actual;
> +
> +  nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
> +  if (!nb)
> +    {
> +      grub_netbuff_free (nb);

This grub_netbuff_free() call is not needed.

> +GRUB_MOD_INIT (ubootnet)
> +{
> +  int devcount, i;
> +  int nfound = 0;
> +
> +  devcount = uboot_dev_enum ();
> +
> +  for (i = 0; i < devcount; i++)
> +    {
> +      struct device_info *devinfo = uboot_dev_get (i);
> +      struct ubootnet_data *ubdata;
> +      struct grub_net_card *card;
> +
> +      if (!(devinfo->type & DEV_TYP_NET))
> +     continue;
> +      
> +      ubdata = grub_malloc (sizeof (struct ubootnet_data));
> +      if (!ubdata)
> +     {
> +       grub_print_error ();
> +       return;
> +     }
> +      card = grub_zalloc (sizeof (struct grub_net_card));
> +      if (!card)
> +     {
> +       grub_free (ubdata);
> +       grub_print_error ();
> +       return;
> +     }
> +
> +      ubdata->handle = i;
> +      ubdata->cookie = devinfo->cookie;
> +
> +      /* FIXME: Any way to check this?  */
> +      card->mtu = 1500;
> +
> +      grub_memcpy (&(card->default_address.mac), &devinfo->di_net.hwaddr, 6);
> +      card->default_address.type = GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET;
> +
> +      card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
> +      card->txbuf = grub_zalloc (card->txbufsize);
> +      if (!card->txbuf)
> +     {
> +       grub_print_error ();
> +       continue;

If this happens, there would be a memory leak since ubdata and card
aren't freed.

> +     }
> +
> +      card->data = ubdata;
> +      card->flags = 0;
> +      card->name = grub_xasprintf ("ubnet_%d", ++nfound);
> +      card->idle_poll_delay_ms = 10;
> +
> +      card->driver = &ubootnet;
> +      grub_net_card_register (card);
> +    }
> +}
> +
> +void
> +grub_ubootdisk_fini (void)
> +{
> +  struct grub_net_card *card, *next;
> +
> +  FOR_NET_CARDS_SAFE (card, next) 
> +    if (card->driver && grub_strcmp (card->driver->name, "ubnet") == 0)
> +      grub_net_card_unregister (card);
> +}

card->txbuf, card->data and card itself should be freed as well.

--
Francesco