Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel

[Jason A. Donenfeld]

Hi Jiri,

On Sat, Oct 6, 2018 at 9:03 AM Jiri Pirko <jiri@xxxxxxxxxxx> wrote:
> >+
> >+      wg->incoming_handshakes_worker =
> >+              wg_packet_alloc_percpu_multicore_worker(
> >+                              wg_packet_handshake_receive_worker, wg);
> >+      if (!wg->incoming_handshakes_worker)
> >+              goto error_2;
>
>
> Please consider renaming the label to "what went wrong". In this case,
> it would be "err_alloc_worker".
>
>
> >+
> >+      wg->handshake_receive_wq = alloc_workqueue("wg-kex-%s",
> >+                      WQ_CPU_INTENSIVE | WQ_FREEZABLE, 0, dev->name);
> >+      if (!wg->handshake_receive_wq)
> >+              goto error_3;
> >+
> >+      wg->handshake_send_wq = alloc_workqueue("wg-kex-%s",
> >+                      WQ_UNBOUND | WQ_FREEZABLE, 0, dev->name);
> >+      if (!wg->handshake_send_wq)
> >+              goto error_4;
> >+
> >+      wg->packet_crypt_wq = alloc_workqueue("wg-crypt-%s",
> >+                      WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM, 0, dev->name);
> >+      if (!wg->packet_crypt_wq)
> >+              goto error_5;
> >+
> >+      if (wg_packet_queue_init(&wg->encrypt_queue, wg_packet_encrypt_worker,
> >+                               true, MAX_QUEUED_PACKETS) < 0)
>
> You need to have "int err" and always in cases like this to do:
> err = wg_packet_queue_init()
> if (err)
>         goto err_*
>
>
> >+              goto error_6;
> >+
> >+      if (wg_packet_queue_init(&wg->decrypt_queue, wg_packet_decrypt_worker,
> >+                               true, MAX_QUEUED_PACKETS) < 0)
> >+              goto error_7;
> >+
> >+      ret = wg_ratelimiter_init();
> >+      if (ret < 0)
> >+              goto error_8;
> >+
> >+      ret = register_netdevice(dev);
> >+      if (ret < 0)
> >+              goto error_9;
> >+
> >+      list_add(&wg->device_list, &device_list);
> >+
> >+      /* We wait until the end to assign priv_destructor, so that
> >+       * register_netdevice doesn't call it for us if it fails.
> >+       */
> >+      dev->priv_destructor = destruct;
> >+
> >+      pr_debug("%s: Interface created\n", dev->name);
> >+      return ret;
> >+
> >+error_9:
> >+      wg_ratelimiter_uninit();
> >+error_8:
> >+      wg_packet_queue_free(&wg->decrypt_queue, true);
> >+error_7:
> >+      wg_packet_queue_free(&wg->encrypt_queue, true);
> >+error_6:
> >+      destroy_workqueue(wg->packet_crypt_wq);
> >+error_5:
> >+      destroy_workqueue(wg->handshake_send_wq);
> >+error_4:
> >+      destroy_workqueue(wg->handshake_receive_wq);
> >+error_3:
> >+      free_percpu(wg->incoming_handshakes_worker);
> >+error_2:
> >+      free_percpu(dev->tstats);
> >+error_1:
> >+      return ret;
> >+}

I'll change away from using error_9,8,7,6,5,4,3,2,1 because of your
suggestion -- and because it's the norm in the kernel to use real
names. But, I would be interested in your opinion on the numerical
errors' reasoning for existing in the first place. The idea was that
with so many different failure cases that need to cascade in the
correct order, it's much easier to visually inspect that it's been
done right by observing up top 1,2,3,4,5,6,7,8,9 and at the bottom
9,8,7,6,5,4,3,2,1, rather than having to store in my brain's limited
stack space what each name pertains to and keep track of the ordering
and such. In light of that, do you still think that following the
convention of textual error labels is a good match here? Again, I'm
changing this for v8, but I am nonetheless curious about what you
think.

Jason