On 8/15/24 3:03 PM, Alexandra Winter wrote:
On 15.08.24 08:43, D. Wythe wrote:
On 8/15/24 11:15 AM, Jeongjun Park wrote:
2024년 8월 15일 (목) 오전 11:51, D. Wythe <alibuda@xxxxxxxxxxxxxxxxx>님이 작성:
On 8/14/24 11:05 PM, Jeongjun Park wrote:
Alexandra Winter wrote:
On 14.08.24 15:11, D. Wythe wrote:
struct smc_sock { /* smc sock container */
- struct sock sk;
+ union {
+ struct sock sk;
+ struct inet_sock inet;
+ };
I don't see a path where this breaks, but it looks risky to me.
Is an smc_sock always an inet_sock as well? Then can't you go with smc_sock->inet_sock->sk ?
Or only in the IPPROTO SMC case, and in the AF_SMC case it is not an inet_sock?
There is no smc_sock->inet_sock->sk before. And this part here was to
make smc_sock also
be an inet_sock.
For IPPROTO_SMC, smc_sock should be an inet_sock, but it is not before.
So, the initialization of certain fields
in smc_sock(for example, clcsk) will overwrite modifications made to the
inet_sock part in inet(6)_create.
For AF_SMC, the only problem is that some space will be wasted. Since
AF_SMC don't care the inet_sock part.
However, make the use of sock by AF_SMC and IPPROTO_SMC separately for
the sake of avoid wasting some space
is a little bit extreme.
Thank you for the explanation D. Wythe. That was my impression also.
I think it is not very clean and risky to use the same structure (smc_sock)
as inet_sock for IPPROTO_SMC and as smc_sock type for AF_SMC.
I am not concerned about wasting space, mroe about maintainability.
Hi Alexandra,
I understand your concern, the maintainability is of course the most
important. But if we use different
sock types for IPPROTO_SMC and AF_SMC, it would actually be detrimental
to maintenance because
we have to use a judgment of which type of sock is to use in all the
code of smc, it's really dirty.
In fact, because a sock is either given to IPPROTO_SMC as inet_sock or
to AF_SMC as smc_sock,
it cannot exist the same time. So it's hard to say what risks there are.
Of course, I have to say that this may not be that clean, but compared
to adding a type judgment
for every sock usage, it is already a very clean approach.
Best wishes,
D. Wythe
Okay. I think using inet_sock instead of sock is also a good idea, but I
understand for now.
However, for some reason this patch status has become Changes Requested
Afaiu, changes requested in this case means that there is discussion ongoing.
, so we will split the patch into two and resend the v5 patch.
Regards,
Jeongjun Park
Why so hurry ? Are you rushing for some tasks ? Please be patient.
The discussion is still ongoing, and you need to wait for everyone's opinions,
at least you can wait a few days to see if there are any other opinions, even if you think
your patch is correct.
[...]
Best wishes,
D. Wythe
I understand that we have a real problem and need a fix. But I agree with D. Wythe,
please give people a chance for discussion before sending new versions.
Also a version history would be helpful (what changed and why)
hmm... then how about changing it to something like this?
@@ -283,7 +283,7 @@ struct smc_connection {
};
struct smc_sock { /* smc sock container */
- struct sock sk;
+ struct inet_sock inet;
struct socket *clcsock; /* internal tcp socket */
void (*clcsk_state_change)(struct sock *sk);
Don't.
/* original stat_change fct. */
@@ -327,7 +327,7 @@ struct smc_sock { /* smc sock container */
* */
};
-#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
+#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, inet.sk)
static inline void smc_init_saved_callbacks(struct smc_sock *smc)
{
It is definitely not normal to make the first member of smc_sock as sock.
Therefore, I think it would be appropriate to modify it to use inet_sock
as the first member like other protocols (sctp, dccp) and access sk in a
way like &smc->inet.sk.
Although this fix would require more code changes, we tested the bug and
confirmed that it was not triggered and the functionality was working
normally.
What do you think?
Yes, that looks like what I had in mind.
I am not familiar enough with the details of the SMC code to judge all implications.
Regards,
Jeongjun Park
