On 6/26/2023 7:12 AM, Dan Carpenter wrote:
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?
"smb" would be fine, but honestly it's only referenced in one
place and used to be static inline, so it kind of doesn't matter
what it's called. It's also unclear to me why it's being moved
to a .c file and made visible.
Anything but "cifs" though.
Tom.
+/*
+ * 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
