First of all it is not as complex as it seems, it does the very same multiple times.
But I still found some things I need to ask.
Overall, once you had a look at those 4 points, addressed or explained them the next sponsor coming by (or myself if I do not miss the ping due to your reply) could sponsor that to Oracular and for the Noble SRU.
#1
I saw the context changed around
PTHREAD_RWLOCK_destroy(&my_fd->fdlock);
which formerly was in many *_free_state functions in the version that is in noble.
That is part of the removing of all custom free functions used as function pointer in the ops struct.
What is left is that instead of through the function pointer like
op_ctx->fsal_export->exp_ops.free_state(
it now calls directly
free_state(pfid->state);
And in these calls the right state is selected that needs to be processed.
The new free_state now checks for bad state.
Then only if a custom state free function is registered calls that or to gsh_free as final fallback.
Those are all set to NULL as seen in `grep -Hrn init_state src/* -A1`.
So gsh_free it is which is in src/include/abstract_mem.h
But that is just:
254 static inline void
255 gsh_free(void *p)
256 {
257 »···free(p);
258 }
Maybe I just do not see it yet, but could you explain why the proposed changes would not break the locking which seems to be no more done?
I'm sure the SRU team would want to know as well before accepting.
#2
Further there is a comment lost in the struct of *_state_fd
Upstream added:
/** state MUST be first to use default free_state */
The comment is not functional and therefore not a huge problem.
But I also see no reason why it is not added, it would only apply other backports harder right?
If there is a reason why that isn't added let explain it.
And if there is none the next revision could have that change to be more similar to the upstream patch right?
#3 the aforementioned versioning and changelog
I think this should be
3.1 oracular
nfs-ganesha (4.3-8ubuntu2) oracular; urgency=medium
#4 dep-3 headers
As mentioned I'm happy you have them.
But since I came to the point to ask you to refresh your proposal anyway.
On one hand since it is adapted we'd change origin->backport
And on the other I see no reason why we should not use exactly the text from the upstream patch.
Was there a reason to reformat it?
I'd suggest starting it with
Patch review
First of all it is not as complex as it seems, it does the very same multiple times.
But I still found some things I need to ask.
Overall, once you had a look at those 4 points, addressed or explained them the next sponsor coming by (or myself if I do not miss the ping due to your reply) could sponsor that to Oracular and for the Noble SRU.
#1 RWLOCK_ destroy( &my_fd- >fdlock) ;
I saw the context changed around
PTHREAD_
which formerly was in many *_free_state functions in the version that is in noble.
That is part of the removing of all custom free functions used as function pointer in the ops struct. >fsal_export- >exp_ops. free_state( state(pfid- >state) ;
What is left is that instead of through the function pointer like
op_ctx-
it now calls directly
free_
And in these calls the right state is selected that needs to be processed.
The new free_state now checks for bad state. abstract_ mem.h
Then only if a custom state free function is registered calls that or to gsh_free as final fallback.
Those are all set to NULL as seen in `grep -Hrn init_state src/* -A1`.
So gsh_free it is which is in src/include/
But that is just:
254 static inline void
255 gsh_free(void *p)
256 {
257 »···free(p);
258 }
Maybe I just do not see it yet, but could you explain why the proposed changes would not break the locking which seems to be no more done?
I'm sure the SRU team would want to know as well before accepting.
#2
Further there is a comment lost in the struct of *_state_fd
Upstream added:
/** state MUST be first to use default free_state */
The comment is not functional and therefore not a huge problem.
But I also see no reason why it is not added, it would only apply other backports harder right?
If there is a reason why that isn't added let explain it.
And if there is none the next revision could have that change to be more similar to the upstream patch right?
#3 the aforementioned versioning and changelog
I think this should be
3.1 oracular
nfs-ganesha (4.3-8ubuntu2) oracular; urgency=medium
* Fix null pointer ref in free_state (LP: #2065856) fix-null- ptr-ref- in-free- state.diff
- d/p/20-
3.2 noble
nfs-ganesha (4.3-8ubuntu1.1) noble; urgency=medium
* Fix null pointer ref in free_state (LP: #2065856) fix-null- ptr-ref- in-free- state.diff
- d/p/20-
#4 dep-3 headers
As mentioned I'm happy you have them.
But since I came to the point to ask you to refresh your proposal anyway.
On one hand since it is adapted we'd change origin->backport
And on the other I see no reason why we should not use exactly the text from the upstream patch.
Was there a reason to reformat it?
I'd suggest starting it with
Description: free_state may not always be able to be called with a valid export /github. com/nfs- ganesha/ nfs-ganesha/ commit/ 336abcba0b9e0da e0aadb4657c311d 04862f2028 /github. com/nfs- ganesha/ nfs-ganesha/ issues/ 904 /launchpad. net/bugs/ 2065856
Instead of trying to use the export attached to a state to call
the FSAL free_state, use a function pointer which if non-NULL
is used, otherwise we just gsh_free the state (which works for all
in-tree FSALs).
Origin: backport, https:/
Bug: https:/
Bug-Ubuntu: https:/
Author: Frank S. Filz <email address hidden>
Reviewed-By: Frank S. Filz <email address hidden>
Last-Update: 2024-05-16
---