Re: [Minios-devel] [PATCH] Save/Restore Support: Fix defects introduced to MiniOS.

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Hello,

Bruno Alvisio, le mer. 04 juil. 2018 10:57:16 -0700, a ecrit:
> @@ -321,6 +321,7 @@ struct netfront_dev *init_netfront(char *_nodename,
>          if (strcmp(nodename, list->dev->nodename) == 0) {
>              list->refcount++;
>              dev = list->dev;
> +            ldev = list;
>              if (thenetif_rx)
>                  netfront_set_rx_handler(dev, thenetif_rx, NULL);
>              goto out;

That looks odd: ldev is not used at all until being filled with a
malloc.

> @@ -474,8 +475,8 @@ again:
>      err = xenbus_transaction_end(xbt, 0, &retry);
>      free(err);
>      if (retry) {
> +        printk("retrying transaction\n");
>          goto again;
> -        printk("completing transaction\n");
>      }
>  
>      goto done;

Uh, indeed, long-time issue :)

> @@ -489,8 +490,10 @@ abort_transaction:
>  done:
>      snprintf(path, sizeof(path), "%s/backend", dev->nodename);
>      msg = xenbus_read(XBT_NIL, path, &dev->backend);
> +    free(msg);
>      snprintf(path, sizeof(path), "%s/mac", dev->nodename);
>      msg = xenbus_read(XBT_NIL, path, &dev->mac);
> +    free(msg);
>  
>      if ((dev->backend == NULL) || (dev->mac == NULL)) {
>          printk("%s: backend/mac failed\n", __func__);

Indeed, but they should be printed before freeing.

> @@ -384,7 +385,7 @@ static struct netfront_dev *_init_netfront(struct 
> netfront_dev *dev,
>                                          char **ip)
>  {
>      xenbus_transaction_t xbt;
> -    char* err = NULL;
> +    char* err = NULL, *err2;
>      char* message=NULL;
>      struct netif_tx_sring *txs;
>      struct netif_rx_sring *rxs;
> @@ -513,13 +516,15 @@ done:
>              err = xenbus_wait_for_state_change(path, &state, &dev->events);
>          if (state != XenbusStateConnected) {
>              printk("backend not avalable, state=%d\n", state);
> -            xenbus_unwatch_path_token(XBT_NIL, path, path);
> +            err2 = xenbus_unwatch_path_token(XBT_NIL, path, path);
> +            free(err2);
>              goto error;
>          }

Rather than introducing an err2 variable, just free(err) after the while
loop, and use err here, and it will be freed by the error: label.

>  
>          if (ip) {
>              snprintf(path, sizeof(path), "%s/ip", dev->backend);
> -            xenbus_read(XBT_NIL, path, ip);
> +            msg = xenbus_read(XBT_NIL, path, ip);
> +            free(msg);


Similarly, better print the error message before freeing it.

> @@ -584,8 +588,6 @@ void shutdown_netfront(struct netfront_dev *dev)
>      list->refcount--;
>      if (list->refcount == 0) {
>          _shutdown_netfront(dev);
> -        free(dev->nodename);
> -        free(dev);
>  
>          to_del = list;
>          if (to_del == dev_list) {

I don't understand why not freeing them?

> diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> index d72dc3a..695c24d 100644
> --- a/xenbus/xenbus.c
> +++ b/xenbus/xenbus.c
> @@ -413,8 +413,11 @@ void resume_xenbus(int canceled)
>  
>              rep = xenbus_msg_reply(XS_WATCH, XBT_NIL, req, ARRAY_SIZE(req));
>              msg = errmsg(rep);
> -            if (msg)
> +            if (msg) {
>                  xprintk("error on XS_WATCH: %s\n", msg);
> +                free(msg);
> +                return;

Why returning? We'd better continue reintroducing the watches which do
succeed.

> +            }
>              free(rep);
>          }
>      }

Samuel

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel