Re: [PATCH net-next v2 11/12] net: homa: create homa_plumbing.c homa_utils.c

[John Ousterhout <ouster@cs.stanford.edu>]

On Mon, Nov 25, 2024 at 9:32 PM D. Wythe <alibuda@linux.alibaba.com> 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-