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
next prev parent reply other threads:[~2018-10-10 9:19 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-06 2:56 [PATCH net-next v7 00/28] WireGuard: Secure Network Tunnel Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 01/28] ARM: makefile: use ARMv3M mode for RiscPC Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 02/28] asm: simd context helper API Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 03/28] zinc: introduce minimal cryptography library Jason A. Donenfeld
2018-10-08 23:22 ` Eric Biggers
2018-10-06 2:56 ` [PATCH net-next v7 04/28] zinc: ChaCha20 generic C implementation and selftest Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 05/28] zinc: import Andy Polyakov's ChaCha20 x86_64 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 06/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 07/28] zinc: import Andy Polyakov's ChaCha20 ARM and ARM64 implementations Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 08/28] zinc: port " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 09/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 10/28] zinc: ChaCha20 MIPS32r2 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 11/28] zinc: Poly1305 generic C implementations and selftest Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 12/28] zinc: import Andy Polyakov's Poly1305 x86_64 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 13/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 14/28] zinc: import Andy Polyakov's Poly1305 ARM and ARM64 implementations Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 15/28] zinc: " Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 16/28] zinc: import Andy Polyakov's Poly1305 MIPS64 implementation Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 17/28] zinc: Poly1305 MIPS32r2 and MIPS64 implementations Jason A. Donenfeld
2018-10-06 2:56 ` [PATCH net-next v7 18/28] zinc: ChaCha20Poly1305 construction and selftest Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 19/28] zinc: BLAKE2s generic C implementation " Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 20/28] zinc: BLAKE2s x86_64 implementation Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 21/28] zinc: Curve25519 generic C implementations and selftest Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 22/28] zinc: Curve25519 x86_64 implementation Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 23/28] zinc: import Bernstein and Schwabe's Curve25519 ARM implementation Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 24/28] zinc: " Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 25/28] crypto: port Poly1305 to Zinc Jason A. Donenfeld
2018-10-08 23:21 ` Eric Biggers
2018-10-09 0:02 ` Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 26/28] crypto: port ChaCha20 " Jason A. Donenfeld
2018-10-06 13:07 ` Martin Willi
2018-10-06 2:57 ` [PATCH net-next v7 27/28] security/keys: rewrite big_key crypto to use Zinc Jason A. Donenfeld
2018-10-06 2:57 ` [PATCH net-next v7 28/28] net: WireGuard secure network tunnel Jason A. Donenfeld
2018-10-06 6:58 ` Jiri Pirko
2018-10-06 12:25 ` Jason A. Donenfeld
2018-10-07 17:26 ` Jason A. Donenfeld
2018-10-07 18:14 ` Lukas Wunner
2018-10-07 18:42 ` Andrew Lunn
2018-10-10 9:13 ` Jiri Pirko [this message]
2018-10-10 20:27 ` Jason A. Donenfeld
2018-10-11 5:36 ` Jiri Pirko
2018-10-06 19:43 ` Eugene Syromiatnikov
2018-10-08 1:06 ` Jason A. Donenfeld
2018-10-06 23:54 ` Andrew Lunn
2018-10-07 1:57 ` Jason A. Donenfeld
2018-10-07 16:48 ` Andrew Lunn
2018-10-07 16:55 ` Jason A. Donenfeld
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181010091358.GB5027@nanopsycho.orion \
--to=jiri@resnulli.us \
--cc=Jason@zx2c4.com \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).