[PATCH] a few cleanups
Jeremy Allison
jra at samba.org
Mon Dec 14 19:02:34 UTC 2015
On Mon, Dec 14, 2015 at 05:14:52PM +0100, Volker Lendecke wrote:
> Hi!
>
> Review appreciated!
Nice cleanup ! Thanks. Pushed.
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de
> From 116eef572995df24d3dcd1d0bda6aff7fe5574fc Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sun, 13 Dec 2015 15:14:18 +0100
> Subject: [PATCH 1/6] lib: Add gencache.h
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/include/proto.h | 21 +-------------------
> source3/lib/gencache.h | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 54 insertions(+), 20 deletions(-)
> create mode 100644 source3/lib/gencache.h
>
> diff --git a/source3/include/proto.h b/source3/include/proto.h
> index 7d0ba42..eeee402 100644
> --- a/source3/include/proto.h
> +++ b/source3/include/proto.h
> @@ -88,26 +88,7 @@ int map_errno_from_nt_status(NTSTATUS status);
>
> struct file_id vfs_file_id_from_sbuf(connection_struct *conn, const SMB_STRUCT_STAT *sbuf);
>
> -/* The following definitions come from lib/gencache.c */
> -
> -bool gencache_set(const char *keystr, const char *value, time_t timeout);
> -bool gencache_del(const char *keystr);
> -bool gencache_get(const char *keystr, TALLOC_CTX *mem_ctx, char **value,
> - time_t *ptimeout);
> -bool gencache_parse(const char *keystr,
> - void (*parser)(time_t timeout, DATA_BLOB blob,
> - void *private_data),
> - void *private_data);
> -bool gencache_get_data_blob(const char *keystr, TALLOC_CTX *mem_ctx,
> - DATA_BLOB *blob,
> - time_t *timeout, bool *was_expired);
> -bool gencache_stabilize(void);
> -bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob, time_t timeout);
> -void gencache_iterate_blobs(void (*fn)(const char *key, DATA_BLOB value,
> - time_t timeout, void *private_data),
> - void *private_data, const char *pattern);
> -void gencache_iterate(void (*fn)(const char* key, const char *value, time_t timeout, void* dptr),
> - void* data, const char* keystr_pattern);
> +#include "lib/gencache.h"
>
> /* The following definitions come from lib/interface.c */
>
> diff --git a/source3/lib/gencache.h b/source3/lib/gencache.h
> new file mode 100644
> index 0000000..61716c4
> --- /dev/null
> +++ b/source3/lib/gencache.h
> @@ -0,0 +1,53 @@
> +/*
> + * Unix SMB/CIFS implementation.
> + *
> + * Generic, persistent and shared between processes cache mechanism for use
> + * by various parts of the Samba code
> + *
> + * Copyright (C) Rafal Szczesniak 2002
> + * Copyright (C) Volker Lendecke 2009
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __LIB_GENCACHE_H__
> +#define __LIB_GENCACHE_H__
> +
> +#include "replace.h"
> +#include "system/time.h"
> +#include "lib/util/data_blob.h"
> +#include "libcli/util/ntstatus.h"
> +
> +bool gencache_set(const char *keystr, const char *value, time_t timeout);
> +bool gencache_del(const char *keystr);
> +bool gencache_get(const char *keystr, TALLOC_CTX *mem_ctx, char **value,
> + time_t *ptimeout);
> +bool gencache_parse(const char *keystr,
> + void (*parser)(time_t timeout, DATA_BLOB blob,
> + void *private_data),
> + void *private_data);
> +bool gencache_get_data_blob(const char *keystr, TALLOC_CTX *mem_ctx,
> + DATA_BLOB *blob,
> + time_t *timeout, bool *was_expired);
> +bool gencache_stabilize(void);
> +bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
> + time_t timeout);
> +void gencache_iterate_blobs(void (*fn)(const char *key, DATA_BLOB value,
> + time_t timeout, void *private_data),
> + void *private_data, const char *pattern);
> +void gencache_iterate(void (*fn)(const char* key, const char *value,
> + time_t timeout, void* dptr),
> + void* data, const char* keystr_pattern);
> +
> +#endif
> --
> 1.9.1
>
>
> From cf70198e227fe210150af12eaaee05f7def52b00 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sun, 13 Dec 2015 15:17:27 +0100
> Subject: [PATCH 2/6] gencache: True->true, False->false
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/gencache.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
> index 90eafaa..ed78476 100644
> --- a/source3/lib/gencache.c
> +++ b/source3/lib/gencache.c
> @@ -63,7 +63,9 @@ static bool gencache_init(void)
> int open_flags = O_RDWR|O_CREAT;
>
> /* skip file open if it's already opened */
> - if (cache) return True;
> + if (cache) {
> + return true;
> + }
>
> cache_fname = cache_path("gencache.tdb");
> if (cache_fname == NULL) {
> @@ -112,7 +114,7 @@ static bool gencache_init(void)
>
> if (!cache) {
> DEBUG(5, ("Attempt to open gencache.tdb has failed.\n"));
> - return False;
> + return false;
> }
>
> cache_fname = lock_path("gencache_notrans.tdb");
> @@ -139,7 +141,7 @@ static bool gencache_init(void)
> }
> TALLOC_FREE(cache_fname);
>
> - return True;
> + return true;
> }
>
> static TDB_DATA last_stabilize_key(void)
> @@ -295,7 +297,9 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
> return false;
> }
>
> - if (!gencache_init()) return False;
> + if (!gencache_init()) {
> + return false;
> + }
>
> if (gencache_have_val(keystr, blob, timeout)) {
> DEBUG(10, ("Did not store value for %s, we already got it\n",
> @@ -305,7 +309,7 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
>
> val = talloc_asprintf(talloc_tos(), CACHE_DATA_FMT, (int)timeout);
> if (val == NULL) {
> - return False;
> + return false;
> }
> val = talloc_realloc(NULL, val, char, talloc_array_length(val)-1);
> if (val == NULL) {
> @@ -383,7 +387,9 @@ bool gencache_del(const char *keystr)
> return false;
> }
>
> - if (!gencache_init()) return False;
> + if (!gencache_init()) {
> + return false;
> + }
>
> DEBUG(10, ("Deleting cache entry (key=[%s])\n", keystr));
>
> @@ -569,7 +575,7 @@ static void gencache_get_data_blob_parser(time_t timeout, DATA_BLOB blob,
> * timeout
> *
> * @retval true when entry is successfuly fetched
> - * @retval False for failure
> + * @retval false for failure
> **/
>
> bool gencache_get_data_blob(const char *keystr, TALLOC_CTX *mem_ctx,
> @@ -604,7 +610,7 @@ bool gencache_get_data_blob(const char *keystr, TALLOC_CTX *mem_ctx,
> *timeout = state.timeout;
> }
>
> - return True;
> + return true;
>
> fail:
> if (was_expired != NULL) {
> @@ -793,14 +799,14 @@ static int wipe_fn(struct tdb_context *tdb, TDB_DATA key, TDB_DATA val,
> * timeout
> *
> * @retval true when entry is successfuly fetched
> - * @retval False for failure
> + * @retval false for failure
> **/
>
> bool gencache_get(const char *keystr, TALLOC_CTX *mem_ctx, char **value,
> time_t *ptimeout)
> {
> DATA_BLOB blob;
> - bool ret = False;
> + bool ret = false;
>
> ret = gencache_get_data_blob(keystr, mem_ctx, &blob, ptimeout, NULL);
> if (!ret) {
> --
> 1.9.1
>
>
> From 92876902dba945b6e6c62d26ab9a5b1bd446c072 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sun, 13 Dec 2015 15:27:15 +0100
> Subject: [PATCH 3/6] lib: Use directory_create_or_exist in xx_path
>
> directory_create_or_exist is a little different: It does the lstat first and
> sets the umask properly, but I think this is more correct than the xx_path()
> version before.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/util.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/source3/lib/util.c b/source3/lib/util.c
> index 1d04318..320fdc2 100644
> --- a/source3/lib/util.c
> +++ b/source3/lib/util.c
> @@ -1515,15 +1515,8 @@ static char *xx_path(const char *name, const char *rootpath)
> }
> trim_string(fname,"","/");
>
> - if (!directory_exist(fname)) {
> - if (mkdir(fname,0755) == -1) {
> - /* Did someone else win the race ? */
> - if (errno != EEXIST) {
> - DEBUG(1, ("Unable to create directory %s for file %s. "
> - "Error was %s\n", fname, name, strerror(errno)));
> - return NULL;
> - }
> - }
> + if (!directory_create_or_exist(fname, 0755)) {
> + return NULL;
> }
>
> return talloc_asprintf_append(fname, "/%s", name);
> --
> 1.9.1
>
>
> From 36e97a4ef0601bcc534c446838d1704af483627b Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sun, 13 Dec 2015 16:32:52 +0100
> Subject: [PATCH 4/6] lib: Separate out xx_path() & callers
>
> We should not have to #include proto.h just for cache_path() or so
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/include/proto.h | 4 +--
> source3/lib/util.c | 65 ---------------------------------
> source3/lib/util_path.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
> source3/lib/util_path.h | 31 ++++++++++++++++
> source3/wscript_build | 1 +
> 5 files changed, 128 insertions(+), 68 deletions(-)
> create mode 100644 source3/lib/util_path.c
> create mode 100644 source3/lib/util_path.h
>
> diff --git a/source3/include/proto.h b/source3/include/proto.h
> index eeee402..1470b6d 100644
> --- a/source3/include/proto.h
> +++ b/source3/include/proto.h
> @@ -400,9 +400,7 @@ int smb_mkstemp(char *name_template);
> void *smb_xmalloc_array(size_t size, unsigned int count);
> char *myhostname(void);
> char *myhostname_upper(void);
> -char *lock_path(const char *name);
> -char *state_path(const char *name);
> -char *cache_path(const char *name);
> +#include "lib/util_path.h"
> bool parent_dirname(TALLOC_CTX *mem_ctx, const char *dir, char **parent,
> const char **name);
> bool ms_has_wild(const char *s);
> diff --git a/source3/lib/util.c b/source3/lib/util.c
> index 320fdc2..303fa8b 100644
> --- a/source3/lib/util.c
> +++ b/source3/lib/util.c
> @@ -1496,71 +1496,6 @@ char *myhostname_upper(void)
> return ret;
> }
>
> -/**
> - * @brief Returns an absolute path to a file concatenating the provided
> - * @a rootpath and @a basename
> - *
> - * @param name Filename, relative to @a rootpath
> - *
> - * @retval Pointer to a string containing the full path.
> - **/
> -
> -static char *xx_path(const char *name, const char *rootpath)
> -{
> - char *fname = NULL;
> -
> - fname = talloc_strdup(talloc_tos(), rootpath);
> - if (!fname) {
> - return NULL;
> - }
> - trim_string(fname,"","/");
> -
> - if (!directory_create_or_exist(fname, 0755)) {
> - return NULL;
> - }
> -
> - return talloc_asprintf_append(fname, "/%s", name);
> -}
> -
> -/**
> - * @brief Returns an absolute path to a file in the Samba lock directory.
> - *
> - * @param name File to find, relative to LOCKDIR.
> - *
> - * @retval Pointer to a talloc'ed string containing the full path.
> - **/
> -
> -char *lock_path(const char *name)
> -{
> - return xx_path(name, lp_lock_directory());
> -}
> -
> -/**
> - * @brief Returns an absolute path to a file in the Samba state directory.
> - *
> - * @param name File to find, relative to STATEDIR.
> - *
> - * @retval Pointer to a talloc'ed string containing the full path.
> - **/
> -
> -char *state_path(const char *name)
> -{
> - return xx_path(name, lp_state_directory());
> -}
> -
> -/**
> - * @brief Returns an absolute path to a file in the Samba cache directory.
> - *
> - * @param name File to find, relative to CACHEDIR.
> - *
> - * @retval Pointer to a talloc'ed string containing the full path.
> - **/
> -
> -char *cache_path(const char *name)
> -{
> - return xx_path(name, lp_cache_directory());
> -}
> -
> /*******************************************************************
> Given a filename - get its directory name
> ********************************************************************/
> diff --git a/source3/lib/util_path.c b/source3/lib/util_path.c
> new file mode 100644
> index 0000000..509ba5f
> --- /dev/null
> +++ b/source3/lib/util_path.c
> @@ -0,0 +1,95 @@
> +/*
> + * Unix SMB/CIFS implementation.
> + * Samba utility functions
> + * Copyright (C) Andrew Tridgell 1992-1998
> + * Copyright (C) Jeremy Allison 2001-2007
> + * Copyright (C) Simo Sorce 2001
> + * Copyright (C) Jim McDonough <jmcd at us.ibm.com> 2003
> + * Copyright (C) James Peach 2006
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "replace.h"
> +#include <talloc.h>
> +#include "lib/util/samba_util.h"
> +#include "lib/util_path.h"
> +
> +struct share_params;
> +#include "source3/param/param_proto.h"
> +
> +/**
> + * @brief Returns an absolute path to a file concatenating the provided
> + * @a rootpath and @a basename
> + *
> + * @param name Filename, relative to @a rootpath
> + *
> + * @retval Pointer to a string containing the full path.
> + **/
> +
> +static char *xx_path(const char *name, const char *rootpath)
> +{
> + char *fname = NULL;
> +
> + fname = talloc_strdup(talloc_tos(), rootpath);
> + if (!fname) {
> + return NULL;
> + }
> + trim_string(fname,"","/");
> +
> + if (!directory_create_or_exist(fname, 0755)) {
> + return NULL;
> + }
> +
> + return talloc_asprintf_append(fname, "/%s", name);
> +}
> +
> +/**
> + * @brief Returns an absolute path to a file in the Samba lock directory.
> + *
> + * @param name File to find, relative to LOCKDIR.
> + *
> + * @retval Pointer to a talloc'ed string containing the full path.
> + **/
> +
> +char *lock_path(const char *name)
> +{
> + return xx_path(name, lp_lock_directory());
> +}
> +
> +/**
> + * @brief Returns an absolute path to a file in the Samba state directory.
> + *
> + * @param name File to find, relative to STATEDIR.
> + *
> + * @retval Pointer to a talloc'ed string containing the full path.
> + **/
> +
> +char *state_path(const char *name)
> +{
> + return xx_path(name, lp_state_directory());
> +}
> +
> +/**
> + * @brief Returns an absolute path to a file in the Samba cache directory.
> + *
> + * @param name File to find, relative to CACHEDIR.
> + *
> + * @retval Pointer to a talloc'ed string containing the full path.
> + **/
> +
> +char *cache_path(const char *name)
> +{
> + return xx_path(name, lp_cache_directory());
> +}
> diff --git a/source3/lib/util_path.h b/source3/lib/util_path.h
> new file mode 100644
> index 0000000..118a4be
> --- /dev/null
> +++ b/source3/lib/util_path.h
> @@ -0,0 +1,31 @@
> +/*
> + * Unix SMB/CIFS implementation.
> + * Samba utility functions
> + * Copyright (C) Andrew Tridgell 1992-1998
> + * Copyright (C) Jeremy Allison 2001-2007
> + * Copyright (C) Simo Sorce 2001
> + * Copyright (C) Jim McDonough <jmcd at us.ibm.com> 2003
> + * Copyright (C) James Peach 2006
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __LIB_UTIL_PATH_H__
> +#define __LIB_UTIL_PATH_H__
> +
> +char *lock_path(const char *name);
> +char *state_path(const char *name);
> +char *cache_path(const char *name);
> +
> +#endif
> diff --git a/source3/wscript_build b/source3/wscript_build
> index e66e89f..d40dd7e 100755
> --- a/source3/wscript_build
> +++ b/source3/wscript_build
> @@ -255,6 +255,7 @@ bld.SAMBA3_SUBSYSTEM('samba3util',
> lib/util_sid.c
> lib/util_file.c
> lib/util.c
> + lib/util_path.c
> lib/util_procid.c
> lib/util_sock.c
> lib/util_tsock.c
> --
> 1.9.1
>
>
> From a09365c8dc586fe070a92c2539ca05b02da9767e Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sun, 13 Dec 2015 21:16:36 +0100
> Subject: [PATCH 5/6] gencache: Refactor gencache_set_data_blob
>
> Replace 3 calls into talloc with 1. Add an overflow check.
>
> With this change, it will be easier to avoid the talloc call for small
> blobs in the future and do it on the stack.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/gencache.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/source3/lib/gencache.c b/source3/lib/gencache.c
> index ed78476..ed16d79 100644
> --- a/source3/lib/gencache.c
> +++ b/source3/lib/gencache.c
> @@ -283,6 +283,8 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
> time_t timeout)
> {
> int ret;
> + fstring hdr;
> + int hdr_len;
> char* val;
> time_t last_stabilize;
> static int writecount;
> @@ -307,19 +309,23 @@ bool gencache_set_data_blob(const char *keystr, const DATA_BLOB *blob,
> return true;
> }
>
> - val = talloc_asprintf(talloc_tos(), CACHE_DATA_FMT, (int)timeout);
> - if (val == NULL) {
> + hdr_len = fstr_sprintf(hdr, CACHE_DATA_FMT, (int)timeout);
> +
> + if (hdr_len == -1) {
> return false;
> }
> - val = talloc_realloc(NULL, val, char, talloc_array_length(val)-1);
> - if (val == NULL) {
> + if ((blob->length + (size_t)hdr_len) < blob->length) {
> return false;
> }
> - val = (char *)talloc_append_blob(NULL, val, *blob);
> +
> + val = talloc_array(talloc_tos(), char, hdr_len + blob->length);
> if (val == NULL) {
> return false;
> }
>
> + memcpy(val, hdr, hdr_len);
> + memcpy(val+hdr_len, blob->data, blob->length);
> +
> DEBUG(10, ("Adding cache entry with key=[%s] and timeout="
> "[%s] (%d seconds %s)\n", keystr,
> timestring(talloc_tos(), timeout),
> --
> 1.9.1
>
>
> From f880856ba18050a6a99badab35d8feb44c481244 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Sun, 13 Dec 2015 21:21:47 +0100
> Subject: [PATCH 6/6] lib: Remove unused talloc_append_blob
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/include/proto.h | 1 -
> source3/lib/util.c | 26 --------------------------
> 2 files changed, 27 deletions(-)
>
> diff --git a/source3/include/proto.h b/source3/include/proto.h
> index 1470b6d..4cd69fd 100644
> --- a/source3/include/proto.h
> +++ b/source3/include/proto.h
> @@ -410,7 +410,6 @@ bool mask_match_search(const char *string, const char *pattern, bool is_case_sen
> bool mask_match_list(const char *string, char **list, int listLen, bool is_case_sensitive);
> bool unix_wild_match(const char *pattern, const char *string);
> bool name_to_fqdn(fstring fqdn, const char *name);
> -void *talloc_append_blob(TALLOC_CTX *mem_ctx, void *buf, DATA_BLOB blob);
> uint32_t map_share_mode_to_deny_mode(uint32_t share_access, uint32_t private_options);
>
> #include "lib/util_procid.h"
> diff --git a/source3/lib/util.c b/source3/lib/util.c
> index 303fa8b..2895c14 100644
> --- a/source3/lib/util.c
> +++ b/source3/lib/util.c
> @@ -1818,32 +1818,6 @@ bool name_to_fqdn(fstring fqdn, const char *name)
> return true;
> }
>
> -/**********************************************************************
> - Append a DATA_BLOB to a talloc'ed object
> -***********************************************************************/
> -
> -void *talloc_append_blob(TALLOC_CTX *mem_ctx, void *buf, DATA_BLOB blob)
> -{
> - size_t old_size = 0;
> - char *result;
> -
> - if (blob.length == 0) {
> - return buf;
> - }
> -
> - if (buf != NULL) {
> - old_size = talloc_get_size(buf);
> - }
> -
> - result = (char *)TALLOC_REALLOC(mem_ctx, buf, old_size + blob.length);
> - if (result == NULL) {
> - return NULL;
> - }
> -
> - memcpy(result + old_size, blob.data, blob.length);
> - return result;
> -}
> -
> uint32_t map_share_mode_to_deny_mode(uint32_t share_access, uint32_t private_options)
> {
> switch (share_access & ~FILE_SHARE_DELETE) {
> --
> 1.9.1
>
More information about the samba-technical
mailing list