Latest leases patchset - getting there !
Jeremy Allison
jra at samba.org
Fri Dec 5 14:17:50 MST 2014
On Fri, Dec 05, 2014 at 02:54:39PM +0100, Stefan (metze) Metzmacher wrote:
> I think we need to change the data model for the
> leases_db.
>
> Maybe something like this:
>
> diff --git a/source3/librpc/idl/leases_db.idl
> b/source3/librpc/idl/leases_db.idl
> index 2ab1591..f6a979d 100644
> --- a/source3/librpc/idl/leases_db.idl
> +++ b/source3/librpc/idl/leases_db.idl
> @@ -15,9 +15,14 @@ interface leases_db
> } leases_db_key;
>
> typedef [public] struct {
> - uint32 num_file_ids;
> - [size_is(num_file_ids)] file_id ids[];
> - [string,charset(UTF8)] char *filename;
> + file_id id;
> + [string,charset(UTF8)] char *servicepath;
> + [string,charset(UTF8)] char *base_name;
> [string,charset(UTF8)] char *stream_name;
> + } leases_db_file;
> +
> + typedef [public] struct {
> + uint32 num_files;
> + [size_is(num_filess)] leases_db_file files[];
> } leases_db_value;
> }
>
> I'm not sure about the servicepath, I took the same values
> as we have in share_mode_data. But we need to maintain the names
> per file_id in order to support dynamic shares.
>
> That way we can check if an open with the same file_id uses
> the correct file name.
Here is an implementation of that data model.
Passes all our smb2.leases tests as well as the
dynamic share test that is in your wip git branch.
I'm hoping this is the last leases change we need
before 4.2.0rc3.
I couldn't figure out how to split out all the
changes and still keep everything compiling inbetween,
so this isn't elegantly bisectable :-(. If you
can figure out how to do that without it being
a blob of a patch, then I'll learn something :-).
Please review and comment !
Cheers,
Jeremy.
-------------- next part --------------
From 4d98af04f63c3a55711a6c9ee7e6da2e74ccd868 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 5 Dec 2014 12:47:52 -0800
Subject: [PATCH 1/5] s3: leases: Change the data model for leases_db to cope
with dynamic path renames.
interface leases_db
{
typedef [public] struct {
GUID client_guid;
smb2_lease_key lease_key;
} leases_db_key;
typedef [public] struct {
file_id id;
[string,charset(UTF8)] char *servicepath;
[string,charset(UTF8)] char *base_name;
[string,charset(UTF8)] char *stream_name;
} leases_db_file;
typedef [public] struct {
uint32 num_files;
[size_is(num_files)] leases_db_file files[];
} leases_db_value;
}
As designed by metze. Unfortunately this commit breaks the compile
until subsequent commits repair the code that uses this new
data model.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/librpc/idl/leases_db.idl | 11 +++++--
source3/locking/leases_db.c | 65 +++++++++++++++++++++++-----------------
source3/locking/leases_db.h | 9 +++---
3 files changed, 51 insertions(+), 34 deletions(-)
diff --git a/source3/librpc/idl/leases_db.idl b/source3/librpc/idl/leases_db.idl
index 2ab1591..d021875 100644
--- a/source3/librpc/idl/leases_db.idl
+++ b/source3/librpc/idl/leases_db.idl
@@ -15,9 +15,14 @@ interface leases_db
} leases_db_key;
typedef [public] struct {
- uint32 num_file_ids;
- [size_is(num_file_ids)] file_id ids[];
- [string,charset(UTF8)] char *filename;
+ file_id id;
+ [string,charset(UTF8)] char *servicepath;
+ [string,charset(UTF8)] char *base_name;
[string,charset(UTF8)] char *stream_name;
+ } leases_db_file;
+
+ typedef [public] struct {
+ uint32 num_files;
+ [size_is(num_files)] leases_db_file files[];
} leases_db_value;
}
diff --git a/source3/locking/leases_db.c b/source3/locking/leases_db.c
index 7e000aa..b428424 100644
--- a/source3/locking/leases_db.c
+++ b/source3/locking/leases_db.c
@@ -85,7 +85,8 @@ static bool leases_db_key(TALLOC_CTX *mem_ctx,
NTSTATUS leases_db_add(const struct GUID *client_guid,
const struct smb2_lease_key *lease_key,
const struct file_id *id,
- const char *filename,
+ const char *servicepath,
+ const char *base_name,
const char *stream_name)
{
TDB_DATA db_key, db_value;
@@ -94,6 +95,7 @@ NTSTATUS leases_db_add(const struct GUID *client_guid,
NTSTATUS status;
bool ok;
struct leases_db_value new_value;
+ struct leases_db_file new_file;
struct leases_db_value *value = NULL;
enum ndr_err_code ndr_err;
@@ -139,31 +141,40 @@ NTSTATUS leases_db_add(const struct GUID *client_guid,
}
/* id must be unique. */
- for (i = 0; i < value->num_file_ids; i++) {
- if (file_id_equal(id, &value->ids[i])) {
+ for (i = 0; i < value->num_files; i++) {
+ if (file_id_equal(id, &value->files[i].id)) {
status = NT_STATUS_OBJECT_NAME_COLLISION;
goto out;
}
}
- value->ids = talloc_realloc(value, value->ids, struct file_id,
- value->num_file_ids + 1);
- if (value->ids == NULL) {
+ value->files = talloc_realloc(value, value->files,
+ struct leases_db_file,
+ value->num_files + 1);
+ if (value->files == NULL) {
status = NT_STATUS_NO_MEMORY;
goto out;
}
- value->ids[value->num_file_ids] = *id;
- value->num_file_ids += 1;
+ value->files[value->num_files].id = *id;
+ value->files[value->num_files].servicepath = servicepath;
+ value->files[value->num_files].base_name = base_name;
+ value->files[value->num_files].stream_name = stream_name;
+ value->num_files += 1;
} else {
DEBUG(10, ("%s: new record\n", __func__));
- new_value = (struct leases_db_value) {
- .num_file_ids = 1,
- .ids = discard_const_p(struct file_id, id),
- .filename = filename,
+ new_file = (struct leases_db_file) {
+ .id = *id,
+ .servicepath = servicepath,
+ .base_name = base_name,
.stream_name = stream_name,
};
+
+ new_value = (struct leases_db_value) {
+ .num_files = 1,
+ .files = &new_file,
+ };
value = &new_value;
}
@@ -252,21 +263,21 @@ NTSTATUS leases_db_del(const struct GUID *client_guid,
}
/* id must exist. */
- for (i = 0; i < value->num_file_ids; i++) {
- if (file_id_equal(id, &value->ids[i])) {
+ for (i = 0; i < value->num_files; i++) {
+ if (file_id_equal(id, &value->files[i].id)) {
break;
}
}
- if (i == value->num_file_ids) {
+ if (i == value->num_files) {
status = NT_STATUS_NOT_FOUND;
goto out;
}
- value->ids[i] = value->ids[value->num_file_ids-1];
- value->num_file_ids -= 1;
+ value->files[i] = value->files[value->num_files-1];
+ value->num_files -= 1;
- if (value->num_file_ids == 0) {
+ if (value->num_files == 0) {
DEBUG(10, ("%s: deleting record\n", __func__));
status = dbwrap_record_delete(rec);
} else {
@@ -303,9 +314,9 @@ NTSTATUS leases_db_del(const struct GUID *client_guid,
}
struct leases_db_fetch_state {
- void (*parser)(uint32_t num_file_ids,
- struct file_id *ids, const char *filename,
- const char *stream_name, void *private_data);
+ void (*parser)(uint32_t num_files,
+ const struct leases_db_file *files,
+ void *private_data);
void *private_data;
NTSTATUS status;
};
@@ -340,8 +351,8 @@ static void leases_db_parser(TDB_DATA key, TDB_DATA data, void *private_data)
NDR_PRINT_DEBUG(leases_db_value, value);
}
- state->parser(value->num_file_ids,
- value->ids, value->filename, value->stream_name,
+ state->parser(value->num_files,
+ value->files,
state->private_data);
TALLOC_FREE(value);
@@ -350,10 +361,8 @@ static void leases_db_parser(TDB_DATA key, TDB_DATA data, void *private_data)
NTSTATUS leases_db_parse(const struct GUID *client_guid,
const struct smb2_lease_key *lease_key,
- void (*parser)(uint32_t num_file_ids,
- struct file_id *ids,
- const char *filename,
- const char *stream_name,
+ void (*parser)(uint32_t num_files,
+ const struct leases_db_file *files,
void *private_data),
void *private_data)
{
@@ -389,6 +398,7 @@ NTSTATUS leases_db_parse(const struct GUID *client_guid,
NTSTATUS leases_db_rename(const struct GUID *client_guid,
const struct smb2_lease_key *lease_key,
const struct file_id *id,
+ const char *servicename_new,
const char *filename_new,
const char *stream_name_new)
{
@@ -404,6 +414,7 @@ NTSTATUS leases_db_rename(const struct GUID *client_guid,
return leases_db_add(client_guid,
lease_key,
id,
+ servicename_new,
filename_new,
stream_name_new);
}
diff --git a/source3/locking/leases_db.h b/source3/locking/leases_db.h
index 906a99b..2e9ab44 100644
--- a/source3/locking/leases_db.h
+++ b/source3/locking/leases_db.h
@@ -24,11 +24,13 @@
struct GUID;
struct smb2_lease_key;
struct file_id;
+struct leases_db_file;
bool leases_db_init(bool read_only);
NTSTATUS leases_db_add(const struct GUID *client_guid,
const struct smb2_lease_key *lease_key,
const struct file_id *id,
+ const char *servicepath,
const char *filename,
const char *stream_name);
NTSTATUS leases_db_del(const struct GUID *client_guid,
@@ -36,15 +38,14 @@ NTSTATUS leases_db_del(const struct GUID *client_guid,
const struct file_id *id);
NTSTATUS leases_db_parse(const struct GUID *client_guid,
const struct smb2_lease_key *lease_key,
- void (*parser)(uint32_t num_file_ids,
- struct file_id *ids,
- const char *filename,
- const char *stream_name,
+ void (*parser)(uint32_t num_files,
+ const struct leases_db_file *files,
void *private_data),
void *private_data);
NTSTATUS leases_db_rename(const struct GUID *client_guid,
const struct smb2_lease_key *lease_key,
const struct file_id *id,
+ const char *servicepath_new,
const char *filename_new,
const char *stream_name_new);
#endif /* _LEASES_DB_H_ */
--
2.2.0.rc0.207.ga3a616c
From e364ab6b541508020c6528675b51479d21acc194 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 5 Dec 2014 12:56:03 -0800
Subject: [PATCH 2/5] s3: leases: Fix rename_share_filename() to use the new
leases_db API.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/locking/locking.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index dd73f68..221d6ee 100644
--- a/source3/locking/locking.c
+++ b/source3/locking/locking.c
@@ -575,6 +575,7 @@ bool rename_share_filename(struct messaging_context *msg_ctx,
status = leases_db_rename(&l->client_guid,
&l->lease_key,
&id,
+ d->servicepath,
d->base_name,
d->stream_name);
if (!NT_STATUS_IS_OK(status)) {
--
2.2.0.rc0.207.ga3a616c
From 6ac2f42246d69dcca53abef3ef66bd6de002fd62 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 5 Dec 2014 12:57:24 -0800
Subject: [PATCH 3/5] s3: leases: Add new utility function
leases_db_copy_file_ids()
Will be used by lease db parsers.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/locking/leases_db.c | 20 ++++++++++++++++++++
source3/locking/leases_db.h | 4 ++++
2 files changed, 24 insertions(+)
diff --git a/source3/locking/leases_db.c b/source3/locking/leases_db.c
index b428424..0700ba9 100644
--- a/source3/locking/leases_db.c
+++ b/source3/locking/leases_db.c
@@ -418,3 +418,23 @@ NTSTATUS leases_db_rename(const struct GUID *client_guid,
filename_new,
stream_name_new);
}
+
+NTSTATUS leases_db_copy_file_ids(TALLOC_CTX *mem_ctx,
+ uint32_t num_files,
+ const struct leases_db_file *files,
+ struct file_id **pp_ids)
+{
+ uint32_t i;
+ struct file_id *ids = talloc_array(mem_ctx,
+ struct file_id,
+ num_files);
+ if (ids == NULL) {
+ return NT_STATUS_NO_MEMORY;
+ }
+
+ for (i = 0; i < num_files; i++) {
+ ids[i] = files[i].id;
+ }
+ *pp_ids = ids;
+ return NT_STATUS_OK;
+}
diff --git a/source3/locking/leases_db.h b/source3/locking/leases_db.h
index 2e9ab44..383575a 100644
--- a/source3/locking/leases_db.h
+++ b/source3/locking/leases_db.h
@@ -48,4 +48,8 @@ NTSTATUS leases_db_rename(const struct GUID *client_guid,
const char *servicepath_new,
const char *filename_new,
const char *stream_name_new);
+NTSTATUS leases_db_copy_file_ids(TALLOC_CTX *mem_ctx,
+ uint32_t num_files,
+ const struct leases_db_file *files,
+ struct file_id **pp_ids);
#endif /* _LEASES_DB_H_ */
--
2.2.0.rc0.207.ga3a616c
From eb13c99618c07ed9b4c453cca65c3ea859aefbd3 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 5 Dec 2014 12:58:39 -0800
Subject: [PATCH 4/5] s3: leases: Fix the lease_parser() in the break code to
use the new data model.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_break.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/source3/smbd/smb2_break.c b/source3/smbd/smb2_break.c
index 126bf78..5eab0a1 100644
--- a/source3/smbd/smb2_break.c
+++ b/source3/smbd/smb2_break.c
@@ -340,21 +340,19 @@ struct lease_lookup_state {
};
static void lease_parser(
- uint32_t num_file_ids,
- struct file_id *ids, const char *filename, const char *stream_name,
+ uint32_t num_files,
+ const struct leases_db_file *files,
void *private_data)
{
struct lease_lookup_state *lls =
(struct lease_lookup_state *)private_data;
lls->status = NT_STATUS_OK;
- lls->num_file_ids = num_file_ids;
- lls->ids = talloc_memdup(lls->mem_ctx,
- ids,
- num_file_ids * sizeof(struct file_id));
- if (lls->ids == NULL) {
- lls->status = NT_STATUS_NO_MEMORY;
- }
+ lls->num_file_ids = num_files;
+ lls->status = leases_db_copy_file_ids(lls->mem_ctx,
+ num_files,
+ files,
+ &lls->ids);
}
static struct tevent_req *smbd_smb2_lease_break_send(
--
2.2.0.rc0.207.ga3a616c
From e7f8565c17bbe3eab6cb506c24a55e9bfd5c6a7b Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 5 Dec 2014 12:59:39 -0800
Subject: [PATCH 5/5] s3: leases: Plumb the new data model into the create
code.
Now we compile again.
Passes smb2.lease.* tests.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/open.c | 153 +++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 121 insertions(+), 32 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 8f19a36..f572a95 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -36,6 +36,7 @@
#include "messages.h"
#include "source3/lib/dbwrap/dbwrap_watch.h"
#include "locking/leases_db.h"
+#include "librpc/gen_ndr/ndr_leases_db.h"
extern const struct generic_mapping file_generic_mapping;
@@ -1677,9 +1678,12 @@ static NTSTATUS grant_fsp_lease(struct files_struct *fsp,
.epoch = fsp->lease->lease.lease_epoch,
};
- status = leases_db_add(client_guid, &lease->lease_key,
- &fsp->file_id, fsp->fsp_name->base_name,
- fsp->fsp_name->stream_name);
+ status = leases_db_add(client_guid,
+ &lease->lease_key,
+ &fsp->file_id,
+ fsp->conn->connectpath,
+ fsp->fsp_name->base_name,
+ fsp->fsp_name->stream_name);
if (!NT_STATUS_IS_OK(status)) {
DEBUG(10, ("%s: leases_db_add failed: %s\n", __func__,
nt_errstr(status)));
@@ -4124,8 +4128,10 @@ static NTSTATUS inherit_new_acl(files_struct *fsp)
* used for a different file name.
*/
-struct lease_fname_match_state {
+struct lease_match_state {
/* Input parameters. */
+ TALLOC_CTX *mem_ctx;
+ const char *servicepath;
const struct smb_filename *fname;
bool file_existed;
struct file_id id;
@@ -4135,57 +4141,139 @@ struct lease_fname_match_state {
NTSTATUS match_status;
};
-static void lease_fname_match_parser(
- uint32_t num_file_ids,
- struct file_id *ids, const char *filename, const char *stream_name,
+/*************************************************************
+ File doesn't exist but this lease key+guid is already in use.
+
+ This is only allowable in the dynamic share case where the
+ service path must be different.
+
+ There is a small race condition here in the multi-connection
+ case where a client sends two create calls on different connections,
+ where the file doesn't exist and one smbd creates the leases_db
+ entry first, but this will get fixed by the multichannel cleanup
+ when all identical client_guids get handled by a single smbd.
+**************************************************************/
+
+static void lease_match_parser_new_file(
+ uint32_t num_files,
+ const struct leases_db_file *files,
+ struct lease_match_state *state)
+{
+ uint32_t i;
+
+ for (i = 0; i < num_files; i++) {
+ const struct leases_db_file *f = &files[i];
+ if (strequal(state->servicepath, f->servicepath)) {
+ state->match_status = NT_STATUS_INVALID_PARAMETER;
+ return;
+ }
+ }
+
+ /* Dynamic share case. Break leases on all other files. */
+ state->match_status = leases_db_copy_file_ids(state->mem_ctx,
+ num_files,
+ files,
+ &state->ids);
+ if (!NT_STATUS_IS_OK(state->match_status)) {
+ return;
+ }
+
+ state->num_file_ids = num_files;
+ state->match_status = NT_STATUS_OPLOCK_NOT_GRANTED;
+ return;
+}
+
+static void lease_match_parser(
+ uint32_t num_files,
+ const struct leases_db_file *files,
void *private_data)
{
- struct lease_fname_match_state *state =
- (struct lease_fname_match_state *)private_data;
+ struct lease_match_state *state =
+ (struct lease_match_state *)private_data;
+ uint32_t i;
- if (!strequal(filename, state->fname->base_name) ||
- !strequal(stream_name, state->fname->stream_name))
- {
- /* Names don't match lease key. */
- state->match_status = NT_STATUS_INVALID_PARAMETER;
+ if (!state->file_existed) {
+ /*
+ * Deal with name mismatch or
+ * possible dynamic share case separately
+ * to make code clearer.
+ */
+ lease_match_parser_new_file(num_files,
+ files,
+ state);
return;
}
- if (state->file_existed &&
- num_file_ids == 1 &&
- file_id_equal(&ids[0],&state->id))
- {
- /* Common case - non-dynamic share. We're ok.. */
- state->match_status = NT_STATUS_OK;
+ /* File existed. */
+ state->match_status = NT_STATUS_OK;
+
+ for (i = 0; i < num_files; i++) {
+ const struct leases_db_file *f = &files[i];
+
+ /* Everything should be the same. */
+ if (!file_id_equal(&state->id, &f->id)) {
+ /* This should catch all dynamic share cases. */
+ state->match_status = NT_STATUS_OPLOCK_NOT_GRANTED;
+ break;
+ }
+ if (!strequal(f->servicepath, state->servicepath)) {
+ state->match_status = NT_STATUS_INVALID_PARAMETER;
+ break;
+ }
+ if (!strequal(f->base_name, state->fname->base_name)) {
+ state->match_status = NT_STATUS_INVALID_PARAMETER;
+ break;
+ }
+ if (!strequal(f->stream_name, state->fname->stream_name)) {
+ state->match_status = NT_STATUS_INVALID_PARAMETER;
+ break;
+ }
+ }
+
+ if (NT_STATUS_IS_OK(state->match_status)) {
+ /*
+ * Common case - just opening another handle on a
+ * file on a non-dynamic share.
+ */
+ return;
+ }
+
+ if (NT_STATUS_EQUAL(state->match_status, NT_STATUS_INVALID_PARAMETER)) {
+ /* Mismatched path. Error back to client. */
return;
}
/*
- * More than one file id, or not equal, or new file
- * being created and there's already an existing lease
- * on this (client_guid, lease id) pair.
+ * File id mismatch. Dynamic share case NT_STATUS_OPLOCK_NOT_GRANTED.
* Don't allow leases.
*/
- state->match_status = NT_STATUS_OPLOCK_NOT_GRANTED;
- state->num_file_ids = num_file_ids;
- state->ids = talloc_memdup(talloc_tos(),
- ids,
- num_file_ids * sizeof(struct file_id));
- if (state->ids == NULL) {
- state->match_status = NT_STATUS_NO_MEMORY;
+ state->match_status = leases_db_copy_file_ids(state->mem_ctx,
+ num_files,
+ files,
+ &state->ids);
+ if (!NT_STATUS_IS_OK(state->match_status)) {
+ return;
}
+
+ state->num_file_ids = num_files;
+ state->match_status = NT_STATUS_OPLOCK_NOT_GRANTED;
+ return;
}
static NTSTATUS lease_match(connection_struct *conn,
struct smb_request *req,
struct smb2_lease_key *lease_key,
+ const char *servicepath,
const struct smb_filename *fname,
uint16_t *p_version,
uint16_t *p_epoch)
{
struct smbd_server_connection *sconn = req->sconn;
- struct lease_fname_match_state state = {
+ TALLOC_CTX *tos = talloc_tos();
+ struct lease_match_state state = {
+ .mem_ctx = tos,
+ .servicepath = servicepath,
.fname = fname,
.match_status = NT_STATUS_OK
};
@@ -4200,7 +4288,7 @@ static NTSTATUS lease_match(connection_struct *conn,
}
status = leases_db_parse(&sconn->client->connections->smb2.client.guid,
- lease_key, lease_fname_match_parser, &state);
+ lease_key, lease_match_parser, &state);
if (!NT_STATUS_IS_OK(status)) {
/*
* Not found or error means okay: We can make the lease pass
@@ -4353,6 +4441,7 @@ static NTSTATUS create_file_unixpath(connection_struct *conn,
status = lease_match(conn,
req,
&lease->lease_key,
+ conn->connectpath,
smb_fname,
&version,
&epoch);
--
2.2.0.rc0.207.ga3a616c
More information about the samba-technical
mailing list