Messages in this thread |  | | Date | Wed, 10 Oct 2018 11:13:58 +0200 | From | Jiri Pirko <> | Subject | Re: [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
|  |