On Fri, Jun 23, 2023 at 12:09:12PM -0400, Tom Talpey wrote:
> On 6/9/2023 1:46 PM, Shyam Prasad N wrote:
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 1250d156619b..9d16626e7a66 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -1288,6 +1288,56 @@ cifs_demultiplex_thread(void *p)
> > module_put_and_kthread_exit(0);
> > }
> > +int
> > +cifs_ipaddr_cmp(struct sockaddr *srcaddr, struct sockaddr *rhs)
>
> Please, please, please, let's not add a new shared entry starting with
> this four-letter word.
>
What would you suggest instead?
> > +/*
> > + * compare two interfaces a and b
> > + * return 0 if everything matches.
> > + * return 1 if a is rdma capable, or rss capable, or has higher link speed
> > + * return -1 otherwise.
> > + */
> > +static int
> > +iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
> > +{
> > + int cmp_ret = 0;
> > +
> > + WARN_ON(!a || !b);
> > + if (a->rdma_capable == b->rdma_capable) {
> > + if (a->rss_capable == b->rss_capable) {
> > + if (a->speed == b->speed) {
> > + cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
> > + (struct sockaddr *) &b->sockaddr);
> > + if (!cmp_ret)
> > + return 0;
> > + else if (cmp_ret > 0)
> > + return 1;
> > + else
> > + return -1;
> > + } else if (a->speed > b->speed)
> > + return 1;
> > + else
> > + return -1;
> > + } else if (a->rss_capable > b->rss_capable)
> > + return 1;
> > + else
> > + return -1;
> > + } else if (a->rdma_capable > b->rdma_capable)
> > + return 1;
> > + else
> > + return -1;
> > +}
> > +
>
> The { <0 / 0 / >0 } behavior of this code has been a source of
> incorrect comparisons in the past, and it still makes my head hurt
> to attempt a review.
>
> So I'll ask, have you thoroughly tested this to be certain that it
> doesn't result in new channels being created needlessly?
I was not a huge fan of this function and the move makes it harder to
review. But I didn't see anything wrong with it.... Here is a slightly
simplified diff that I use to review.
regards,
dan carpenter
iface_cmp(struct cifs_server_iface *a, struct cifs_server_iface *b)
{
int cmp_ret = 0;
WARN_ON(!a || !b);
- if (a->speed == b->speed) {
if (a->rdma_capable == b->rdma_capable) {
if (a->rss_capable == b->rss_capable) {
- cmp_ret = memcmp(&a->sockaddr, &b->sockaddr,
- sizeof(a->sockaddr));
+ if (a->speed == b->speed) {
+ cmp_ret = cifs_ipaddr_cmp((struct sockaddr *) &a->sockaddr,
+ (struct sockaddr *) &b->sockaddr);
if (!cmp_ret)
return 0;
else if (cmp_ret > 0)
return 1;
else
return -1;
- } else if (a->rss_capable > b->rss_capable)
+ } else if (a->speed > b->speed)
return 1;
else
return -1;
- } else if (a->rdma_capable > b->rdma_capable)
+ } else if (a->rss_capable > b->rss_capable)
return 1;
else
return -1;
- } else if (a->speed > b->speed)
+ } else if (a->rdma_capable > b->rdma_capable)
return 1;
else
return -1;
}
regards,
dan carpenter
