lkml.org 
[lkml]   [2018]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
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

\
 
 \ /
  Last update: 2018-10-10 11:20    [W:0.135 / U:0.452 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site