[PATCH] dbwrap_ctdb: Treat empty records as non-existing

[Christian Ambach]

Am 29.08.13 08:09, schrieb Volker Lendecke:
>> 1. make sense
>
> Yes, they do.
>
>> 2. are still necessary?
>
> No, I don't think so. At least in my tests I did not see any
> messages. We now hopefully return NT_STATUS_NOT_FOUND
> whenever we see an empty record in a ctdb-hosted database.
> With that in place we will never hand emtpy TDB_DATA to
> parse_share_modes. If we do, I want to see that message, so
> that we can fix the remaining spots in dbwrap_ctdb.

I still feel that we should add the attached patch.
The absence of level 0 logs does not mean that everything is correct now :)
Imagine the following flow:
1. File gets opened first time => TDB entry gets created
2. File gets closed => destructor will delete the record,
db_ctdb_delete() will simply store it as empty and send deletion request
to CTDB
3. Before vacuuming kicks in, file gets re-opened
4. fetch_share_mode_locked (and also fetch_share_mode_unlocked) will
call parse_share_modes on empty record, leading the code into
parse_share_modes while it should have gone into fresh_share_mode_lock.

I verified that this happens, however I am not sure about the results
and side-effects. The fresh marker will definitely be set to the wrong
value, I am not sure about the importance of initializing the other
members of the struct properly (as fresh_share_mode_lock would do).

I think I saw some nasty logs from NDR in the past when it was asked to
parse such a record, those logs have gone in the meantime, I would
assume due to a change in NDR.

As long as we do not have dbwrap_fetch_locked returning NTSTATUS to be
able to use NT_STATUS_NOT_FOUND, the code would have to look out for
both NULL or dsize == 0.

Cheers,
Christian

-------------- next part --------------
>From 57dd6a422ee77a6d631ac4e4628bdceeaf081208 Mon Sep 17 00:00:00 2001
From: Christian Ambach <ambi at samba.org>
Date: Wed, 28 Aug 2013 23:18:28 +0200
Subject: [PATCH] s3:locking: deal with empty records

when destructing a share mode entry, it might get emptied
and deleted.
In the CTDB case, deleted records will stay in the database
until they get vacuumed. So long they will sit there as empty
records and that causes issues when attempting to parse
the record again on a subsequent open.

Signed-off-by: Christian Ambach <ambi at samba.org>
---
 source3/locking/share_mode_lock.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c
index 0693cf5..4c2376a 100644
--- a/source3/locking/share_mode_lock.c
+++ b/source3/locking/share_mode_lock.c
@@ -123,6 +123,12 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx,
 	enum ndr_err_code ndr_err;
 	DATA_BLOB blob;
 
+
+	/* empty record */
+	if (dbuf.dptr == NULL || dbuf.dsize == 0) {
+		return NULL;
+	}
+
 	d = talloc(mem_ctx, struct share_mode_data);
 	if (d == NULL) {
 		DEBUG(0, ("talloc failed\n"));
@@ -307,7 +313,7 @@ static struct share_mode_lock *get_share_mode_lock_internal(
 
 	value = dbwrap_record_get_value(rec);
 
-	if (value.dptr == NULL) {
+	if (value.dptr == NULL || value.dsize == 0) {
 		d = fresh_share_mode_lock(mem_ctx, servicepath, smb_fname,
 					  old_write_time);
 	} else {
-- 
1.8.1.2