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

From: Jiri Pirko <jiri@resnulli.us>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Netdev <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
Date: Wed, 10 Oct 2018 11:13:58 +0200	[thread overview]
Message-ID: <20181010091358.GB5027@nanopsycho.orion> (raw)
In-Reply-To: <CAHmME9pKvUOaAHhe7gb3yH4jdkdTtLv+gUqdnh3bJZcN776UWQ@mail.gmail.com>

Sun, Oct 07, 2018 at 07:26:47PM CEST, Jason@zx2c4.com wrote:
>Hi Jiri,
>
>On Sat, Oct 6, 2018 at 9:03 AM Jiri Pirko <jiri@resnulli.us> 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.

I think it is perfectly readable when you have:

	err = do_thing_x();
	if (err)
		goto err_do_thing_x;

err_do_thing_x;

Rest of the code (at least in netdev subtree) uses this a lot.

>
>Jason