On Mon, Nov 25, 2024 at 9:32 PM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:
> On 11/12/24 7:40 AM, John Ousterhout wrote:
> > +static struct proto homav6_prot = {
> > ...
> > + .obj_size = sizeof(struct homa_sock) + sizeof(struct ipv6_pinfo),
>
> The implementation of inet6_sk_generic() has already changed, you should set
> .ipv6_pinfo_offset.
Fixed.
> > +static int __init homa_load(void)
> > ...
> > + inet_register_protosw(&homa_protosw);
> > + inet6_register_protosw(&homav6_protosw);
>
>
> better to check the retval of inet6_register_protosw().
Fixed.
> > +out_cleanup:
> > + homa_destroy(homa);
> > + inet_del_protocol(&homa_protocol, IPPROTO_HOMA);
> > + inet_unregister_protosw(&homa_protosw);
> > + inet6_del_protocol(&homav6_protocol, IPPROTO_HOMA);
> > + inet6_unregister_protosw(&homav6_protosw);
> > + proto_unregister(&homa_prot);
> > + proto_unregister(&homav6_prot);
>
>
> It's a bit strange for me that this relies on a premise: that every reverse operation can correctly
> identify whether the corresponding forward operation has been executed. Currently, perhaps every
> function includes this capability. It's up to you, I don't insist.
Actually, not all of the cleanup functions are safe if the initializer
hasn't been invoked; good catch. I've fixed this now.
> Perhaps you can try adding MODULE_ALIAS_NET_PF_PROTO_TYPE so that the kernel will automatically load
> the module when creating IPPROTO_HOMA socket. A functional suggestion, It's up to you.
Done; thanks for the suggestion (I wasn't aware of this feature).
> Is binding multiple times legal? For example, bind 80 first and then bind 8080. If not, I think
> you might need to check the inet_num.
Yes, it's legal.
> I noticed that homa_sock_init() contains a memory allocation action, perhaps you should add a return
> value check.
Oops; I've fixed this now.
Thanks for all the comments.
-John-
