Messages in this thread |  | | From | "Jason A. Donenfeld" <> | Date | Sun, 7 Oct 2018 19:26:47 +0200 | Subject | Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel |
| |
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.
Jason
|  |