Use isc_buffer_t for read/write ops

To support materialization, each value stored inside cfgmgr must have an extra flag telling in which mode the value was inserted/updated (i.e. was it in builtin mode, user mode or running mode). Instead of playing with pointers arithmetic to squeeze this extra info in the value being serialized/deserialized into LMDB, uses isc_buffer_t.
2025-02-24 10:35:39 +01:00
parent a06f88d54a
commit 02482d88ba
3 changed files with 113 additions and 44 deletions
@@ -169,6 +169,13 @@ isc_cfgmgr_init(isc_mem_t *mctx, const char *dbpath) {
 	REQUIRE(mctx != NULL);
 	REQUIRE(dbpath != NULL);

+	/*
+	 * Validate assumptions used when reading/writting data.
+	 */
+	REQUIRE(sizeof(bool) == sizeof(uint8_t));
+	REQUIRE(sizeof(isc_cfgmgr_type_t) == sizeof(uint8_t));
+	REQUIRE(sizeof(isc_cfgmgr_mode_t) == sizeof(uint8_t));
+
 	isc_mem_attach(mctx, &isc__cfgmgr_mctx);
 	INSIST(isc__cfgmgr_mctx != NULL);

@@ -655,12 +662,43 @@ isc_cfgmgr_delnode(void) {
 	isc_cfgmgr_close();
 }

-isc_result_t
-isc_cfgmgr_read(const char *name, isc_cfgmgr_val_t *value) {
+static size_t
+isc__cfgmgr_valuesz(const isc_cfgmgr_val_t *value) {
+	size_t sz = sizeof(value->type);
+
+	sz += sizeof(isc_cfgmgr_mode_t);
+	switch (value->type) {
+	case ISC_CFGMGR_UNDEFINED:
+		UNREACHABLE();
+	case ISC_CFGMGR_STRING:
+		sz += strlen(value->string) + 1;
+		break;
+	case ISC_CFGMGR_BOOLEAN:
+		sz += sizeof(value->boolean);
+		break;
+	case ISC_CFGMGR_NONE:
+		break;
+	case ISC_CFGMGR_SOCKADDR:
+		sz += sizeof(value->sockaddr);
+		break;
+	case ISC_CFGMGR_UINT32:
+		sz += sizeof(value->uint32);
+		break;
+	}
+
+	return sz;
+}
+
+static isc_result_t
+isc__cfgmgr_read(MDB_cursor *cursor, const char *name, isc_cfgmgr_val_t *value,
+		 isc_cfgmgr_mode_t *modep) {
 	isc_result_t result = ISC_R_SUCCESS;
 	MDB_val dbkey;
 	MDB_val dbval;
+	isc_cfgmgr_mode_t mode;
 	char key[BUFLEN];
+	isc_buffer_t b;
+	isc_region_t r;

 	isc__cfgmgr_checkopenednodes();
 	REQUIRE(name != NULL);
@@ -672,34 +710,59 @@ isc_cfgmgr_read(const char *name, isc_cfgmgr_val_t *value) {
 	dbkey = (MDB_val){ .mv_size = isc__cfgmgr_keylen(key) + 1,
 			   .mv_data = key };

-	if (mdb_cursor_get(isc__cfgmgr_lmdbcursor(), &dbkey, &dbval, MDB_SET) !=
-	    0)
-	{
+	if (mdb_cursor_get(cursor, &dbkey, &dbval, MDB_SET) != 0) {
 		result = ISC_R_NOTFOUND;
 		goto out;
 	}

-	const isc_cfgmgr_type_t strtype = ISC_CFGMGR_STRING;
-	if (memcmp(dbval.mv_data, &strtype, sizeof(strtype)) == 0) {
-		value->type = strtype;
-		value->string = (char *)dbval.mv_data + sizeof(strtype);
-	} else {
-		REQUIRE(sizeof(*value) == dbval.mv_size);
-		memmove(value, dbval.mv_data, sizeof(*value));
+	isc_buffer_init(&b, dbval.mv_data, dbval.mv_size);
+	isc_buffer_add(&b, dbval.mv_size);
+
+	value->type = isc_buffer_getuint8(&b);
+	mode = isc_buffer_getuint8(&b);
+	if (modep) {
+		*modep = mode;
 	}

-	INSIST(value->type != ISC_CFGMGR_UNDEFINED);
+	switch (value->type) {
+	case ISC_CFGMGR_UNDEFINED:
+		UNREACHABLE();
+	case ISC_CFGMGR_STRING:
+		isc_buffer_remainingregion(&b, &r);
+		value->string = (const char *)r.base;
+		break;
+	case ISC_CFGMGR_BOOLEAN:
+		value->boolean = isc_buffer_getuint8(&b);
+		break;
+	case ISC_CFGMGR_NONE:
+		break;
+	case ISC_CFGMGR_SOCKADDR:
+		isc_buffer_remainingregion(&b, &r);
+		memmove(&value->sockaddr, r.base, r.length);
+		break;
+	case ISC_CFGMGR_UINT32:
+		value->uint32 = isc_buffer_getuint32(&b);
+		break;
+	}

 out:
 	return result;
 }

+isc_result_t
+isc_cfgmgr_read(const char *name, isc_cfgmgr_val_t *value) {
+	return isc__cfgmgr_read(isc__cfgmgr_lmdbcursor(), name, value, NULL);
+}
+
 isc_result_t
 isc_cfgmgr_write(const char *name, const isc_cfgmgr_val_t *value) {
 	isc_result_t result = ISC_R_SUCCESS;
 	MDB_val dbkey;
 	MDB_val dbval;
 	char key[BUFLEN];
+	isc_buffer_t b;
+	isc_region_t r;
+	char *bdata;

 	isc__cfgmgr_checkopenednodes();
 	REQUIRE(isc__cfgmgr_ctx.readonly == false);
@@ -725,34 +788,39 @@ isc_cfgmgr_write(const char *name, const isc_cfgmgr_val_t *value) {
 		goto out;
 	}

-	/*
-	 * String is a specific case as it needs an allocation, so the size is
-	 * just the size of the type and the string length + 1. For the other
-	 * cases, in order to keep the flow unconditional and simple there is a
-	 * slight memory waste, i.e. storing a uint32_t will take the size of
-	 * isc_cfgmgr_val_t.
-	 */
-	if (value->type == ISC_CFGMGR_STRING) {
-		dbval.mv_size = sizeof(value->type) + strlen(value->string) + 1;
-		dbval.mv_data = isc_mem_allocate(isc__cfgmgr_mctx,
-						 dbval.mv_size);
-		memmove(dbval.mv_data, value, sizeof(value->type));
-		strcpy((char *)dbval.mv_data + sizeof(value->type),
-		       value->string);
-	} else {
-		dbval = (MDB_val){ .mv_size = sizeof(*value),
-				   /*
-				    * LMDB won't modify the mv_data buffer but
-				    * its API is designed w/o the const buffer.
-				    */
-				   .mv_data = (void *)value };
+	dbval.mv_size = isc__cfgmgr_valuesz(value);
+	bdata = isc_mem_allocate(isc__cfgmgr_mctx, dbval.mv_size);
+	isc_buffer_init(&b, bdata, dbval.mv_size);
+	dbval.mv_data = isc_buffer_base(&b);
+
+	isc_buffer_putuint8(&b, value->type);
+	isc_buffer_putuint8(&b, isc__cfgmgr_ctx.mode);
+	switch (value->type) {
+	case ISC_CFGMGR_UNDEFINED:
+		UNREACHABLE();
+	case ISC_CFGMGR_STRING:
+		r.base = (unsigned char *)value->string;
+		r.length = strlen(value->string) + 1;
+		isc_buffer_copyregion(&b, &r);
+		break;
+	case ISC_CFGMGR_BOOLEAN:
+		isc_buffer_putuint8(&b, value->boolean);
+		break;
+	case ISC_CFGMGR_NONE:
+		break;
+	case ISC_CFGMGR_SOCKADDR:
+		r.base = (unsigned char *)&value->sockaddr;
+		r.length = sizeof(value->sockaddr);
+		isc_buffer_copyregion(&b, &r);
+		break;
+	case ISC_CFGMGR_UINT32:
+		isc_buffer_putuint32(&b, value->uint32);
+		break;
 	}

 	REQUIRE(mdb_cursor_put(isc__cfgmgr_lmdbcursor(), &dbkey, &dbval, 0) ==
 		0);
-	if (value->type == ISC_CFGMGR_STRING) {
-		isc_mem_free(isc__cfgmgr_mctx, dbval.mv_data);
-	}
+	isc_mem_free(isc__cfgmgr_mctx, dbval.mv_data);

 out:
 	return result;
@@ -64,7 +64,7 @@ enum isc_cfgmgr_mode {
 	ISC_CFGMGR_MODEBUILTIN,
 	ISC_CFGMGR_MODEUSER,
 	ISC_CFGMGR_MODERUNNING
-};
+} __attribute__((__packed__));

 /*
 * Supported data types for read/write operations from/to cfgmgr.
@@ -82,7 +82,7 @@ typedef enum isc_cfgmgr_type {
 * Generic value holding the actual value and type value for
 * read/write from/to cfgmgr.
 *
- * cfgmgr_type_t::NONE doesn't have associated value,
+ * cfgmgr_type_t::NONE doesn't have associated value.
 */
 typedef struct isc_cfgmgr_val {
 	isc_cfgmgr_type_t type;
@@ -1895,9 +1895,9 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_dumptests) {
 // ISC_CFGMGR_UINT32 }; 	const isc_cfgmgr_prop_t prop2bis = { "prop2bis",
 // false, 					     ISC_CFGMGR_UINT32 };
 // const isc_cfgmgr_prop_t *foo_props[] = { &prop1, &prop2, &prop2bis,
-//NULL }; 	const isc_cfgmgr_clause_t *foo_clauses[] = { &bar, NULL };
-//const isc_cfgmgr_clause_t foo = { "foo", false, false, foo_clauses,
-//foo_props };
+// NULL }; 	const isc_cfgmgr_clause_t *foo_clauses[] = { &bar, NULL };
+// const isc_cfgmgr_clause_t foo = { "foo", false, false, foo_clauses,
+// foo_props };
 //
 //	const isc_cfgmgr_prop_t prop5 = { "prop5", false,
 // ISC_CFGMGR_STRING }; 	const isc_cfgmgr_prop_t *baz_props[] = { &prop5,
@@ -1975,8 +1975,9 @@ ISC_RUN_TEST_IMPL(isc_cfgmgr_dumptests) {
 //					     cfgmgr_dumptests_clausestarts,
 //				     .clauseends =
 // cfgmgr_dumptests_clauseends, 				     .property =
-// cfgmgr_dumptests_property, 				     .state = &buffer, 				     .indent = 8, 				     .dumpmode =
-//ISC_CFGMGR_MODEUSER };
+// cfgmgr_dumptests_property, 				     .state = &buffer,
+// .indent = 8, 				     .dumpmode =
+// ISC_CFGMGR_MODEUSER };
 //
 //	isc_buffer_init(&buffer, data, sizeof(data));
 //	result = isc_cfgmgr_dump(&cfg);