[PATCHES BUG 13446] smbd: Cache dfree information based on query path
Jeremy Allison
jra at samba.org
Wed May 23 23:19:11 UTC 2018
On Wed, May 23, 2018 at 01:37:52PM -0700, Christof Schmitt wrote:
> Here is the updated patch series including a test. The last patch should
> not be backported as it would change the VFS ABI.
>
> Could you review this one?
Firstly, you've got some trailing whitespace (git-hook caught that for me).
In:
uint64_t get_dfree_info(connection_struct *conn, struct smb_filename *fname,
uint64_t *bsize, uint64_t *dfree, uint64_t *dsize)
{
int dfree_cache_time = lp_dfree_cache_time(SNUM(conn));
- struct dfree_cached_info *dfc = conn->dfree_info;
+ struct dfree_cached_info *dfc = NULL, dfc_new;
uint64_t dfree_ret;
+ char tmpbuf[PATH_MAX], *full_path, *to_free, *key_path;
+ size_t len;
+ DATA_BLOB key, value;
....
Please split the new variables onto separate lines and initialize to NULL, e.g.
+ struct dfree_cached_info *dfc = NULL
+ struct dfree_cached_info dfc_new;
+ char tmpbuf[PATH_MAX];
+ char *full_path = NULL;
+ char *to_free = NULL;
+ char *key_path = NULL;
Ensure dfc_new is zeroed before use:
+ DBG_DEBUG("Creating dfree cache entry for %s\n", key_path);
+ ZERO_STRUCT(dfc_new); <-- add this.
+ dfc_new.bsize = *bsize;
+ dfc_new.dfree = *dfree;
+ dfc_new.dsize = *dsize;
This section:
+ if (VALID_STAT(fname->st) && S_ISREG(fname->st.st_ex_mode)) {
+ /*
+ * In case of a file use the parent directory to reduce number
+ * of cache entries.
+ */
+ bool ok;
+
+ ok = parent_dirname(talloc_tos(), full_path, &key_path, NULL);
+ if (!ok) {
+ errno = ENOMEM;
+ return -1;
+ }
+
+ } else {
+ key_path = full_path;
+ }
Made me think. In the 'if (!ok)' error case you need to TALLOC_FREE(to_free),
also, parent_dirname() will *always* allocate key_path, meaning on normal
exit on this case you need to TALOC_FREE(to_free), but you're not freeing key_path.
As you're done with 'to_free' if parent_dirname returns true, you might
want to change this to:
+ if (VALID_STAT(fname->st) && S_ISREG(fname->st.st_ex_mode)) {
+ /*
+ * In case of a file use the parent directory to reduce number
+ * of cache entries.
+ */
+ bool ok;
+
+ ok = parent_dirname(talloc_tos(), full_path, &key_path, NULL);
+ if (!ok) {
+ TALLOC_FREE(to_free);
+ errno = ENOMEM;
+ return -1;
+ }
+ TALLOC_FREE(to_free); /* We're done with full_path */
+ /* key_path is always a talloced object. */
+ to_free = key_path;
+
+ } else {
+ /* key_path might not be a talloced object. */
+ key_path = full_path;
+ }
That way you always exit with only *one* thing to free -> 'to_free'
and you note what paths talloc and what paths might not.
The reason I bring this up is in one path, key_path is a talloced
object, but in the other key_path could be pointing to the stack
allocated char tmpbuf[PATH_MAX] (probably will in fact).
I just worry in the future someone might add a TALLOC_FREE(key_path)
without reading or understanding the code carefully enough.
In the last patch where you move the struct dfree_cached_info *dfree_info,
add a comment to source3/include/vfs.h noting you've changed the VFS ABI,
e.g.
/* Version 39 - Remove SMB_VFS_FSYNC
Only implement async versions. */
/* Version 39 - Remove SMB_VFS_READ
All users are now pread or async versions. */
/* Version 39 - Remove SMB_VFS_WRITE
All users are now pwrite or async versions. */
+ /* Version 39 - Remove struct dfree_cached_info pointer from
+ connection struct */
#define SMB_VFS_INTERFACE_VERSION 39
Otherwise looks good ! Can you re-do those and resend to me,
and I'll push if happy.
Cheers,
Jeremy.
More information about the samba-technical
mailing list