Patches for more python support for registry API

[Jelmer Vernooij]

Hey Wilco, :)

Am 10/05/12 15:13, schrieb Wilco Baan Hofman:
> I've made some patches for the registry library in samba.
>
> One which extends the python registry API to also have key/value
> iteration and traversing to subkeys, getting key values and key
> information.
>
> The second patches the regf backend to return the class name in the unix
> character set, like the other backends do.
>
> Can somebody sign off on these and commit them?
Thanks! Any chance you can add some tests?

This looks good in general; some more comments below:

> @@ -459,10 +460,11 @@ static WERROR regf_get_info(TALLOC_CTX *mem_ctx,
>   		if (private_data->nk->clsname_offset != -1) {
>   			DATA_BLOB data = hbin_get(private_data->hive,
>   						  private_data->nk->clsname_offset);
> -			*classname = talloc_strndup(mem_ctx,
> -						    (char*)data.data,
> -						    private_data->nk->clsname_length);
> -			W_ERROR_HAVE_NO_MEMORY(*classname);
> +			success = convert_string_talloc(mem_ctx, CH_UTF16, CH_UNIX, data.data,
> +			                                private_data->nk->clsname_length,
> +			                                classname, NULL);
> +			if (!success)
> +				return WERR_INVALID_DATATYPE;
>
WERR_INVALID_DATATYPE seems like an odd error in this case.

> +static PyObject *py_hive_get_value(PyObject *self, PyObject *args)
> +{
> +	struct hive_key *key = PyHiveKey_AsHiveKey(self);
> +	const char *name;
> +	DATA_BLOB data;
> +	uint32_t type;
> +	PyObject *value;
> +	PyObject *PyData;
> +	const char *string;
> +	WERROR result;
> +	bool success;
> +
> +	if (!PyArg_ParseTuple(args, "s", &name))
> +		return NULL;
> +
> +	result = key->ops->get_value_by_name(key, key, name, &type, &data);
> +	PyErr_WERROR_IS_ERR_RAISE(result);
> +
> +	switch (type) {
> +		case REG_SZ:
> +		case REG_EXPAND_SZ:
> +		case REG_LINK:
> +			success = convert_string_talloc(key, CH_UTF16, CH_UTF8,
> +			                                data.data, data.length, &string, NULL);
> +			if (!success)
> +				PyErr_SetWERROR(WERR_INVALID_DATATYPE);
Please free data.data on errors.
> +
> +			PyData = PyString_FromString(string);
PyString_FromString can return NULL, in which case you should return 
NULL from here (and free data.data).
> +			break;
> +		case REG_BINARY:
> +		case REG_MULTI_SZ: /* FIXME Could use parsing */
> +		case REG_NONE:
> +			PyData = PyString_FromStringAndSize((char *)data.data, data.length);
PyString_FromStringAndSize can return NULL.
> +			break;
> +		case REG_DWORD:
> +		case REG_DWORD_BIG_ENDIAN:
> +			PyData = PyLong_FromUnsignedLong(*(uint32_t *)data.data);
likewise
> +			break;
> +		case REG_QWORD:
> +			PyData = PyLong_FromUnsignedLong(*(uint64_t *)data.data);
likewise
> +			break;
> +		default:
> +			PyErr_SetWERROR(WERR_INVALID_DATATYPE);
In this case, data.data should be freed.
> +			return NULL;
> +	}
> +
> +	value = PyDict_New();
PYDict_New() can return NULL.
> +	PyDict_SetItemString(value, "type", PyLong_FromUnsignedLong(type));
PyLong_FromUnsignedLong can return NULL
> +	PyDict_SetItemString(value, "data", PyData);
Returning a dictionary in this case is a bit odd. Is there any reason 
for not returning a tuple ? That's a lot more useful to callers.

> +
> +typedef struct {
> +	PyObject_HEAD
> +		uint32_t current_idx;
> +	PyObject *iteratee;
> +} PyValueIteratorObject;
> +
> +
> +static PyObject *py_hive_value_iter_next(PyValueIteratorObject *self) {
Please add the { on a separate line (see the coding style).
> +	struct hive_key *key = PyHiveKey_AsHiveKey(self->iteratee);
> +	WERROR result;
> +	const char *name;
> +
> +	result = key->ops->enum_value(key, key, self->current_idx, &name, NULL, NULL);
> +	if (W_ERROR_EQUAL(result,WERR_NO_MORE_ITEMS))
Please add a space after a comma.
> +		return NULL;
> +	PyErr_WERROR_IS_ERR_RAISE(result);
> +
> +	self->current_idx++;
> +
> +	return PyString_FromString(name);
> +}

> +static PyObject *py_hive_subkey_iter_next(PySubkeyIteratorObject *self) {
Likewise about { on a new line.
> +static PyObject *py_hive_get_key_info(PyObject *self, PyObject *args)
> +{
> +	struct hive_key *key = PyHiveKey_AsHiveKey(self);
> +	WERROR result;
> +	const char *class_name;
> +	uint32_t num_subkeys;
> +	uint32_t num_values;
> +	NTTIME last_change_time;
> +	uint32_t max_subkey_name_len;
> +	uint32_t max_value_name_len;
> +	uint32_t max_value_buf_size;
> +	PyObject *dict;
> +	time_t last_change_time_unix;
> +
> +	result = key->ops->get_key_info(key, key, &class_name, &num_subkeys,
> +			&num_values, &last_change_time, &max_subkey_name_len,
> +			&max_value_name_len, &max_value_buf_size);
> +	PyErr_WERROR_IS_ERR_RAISE(result);
> +
> +
> +	/* Convert the NT time to unix time so it can be used directly with the python time module */
> +	last_change_time_unix = nt_time_to_unix(last_change_time);
> +
> +	dict = PyDict_New();
PyDict_New can return NULL.
> +
> +	PyDict_SetItemString(dict, "class_name", PyString_FromString(class_name));
> +	PyDict_SetItemString(dict, "num_subkeys", PyLong_FromUnsignedLong(num_subkeys));
> +	PyDict_SetItemString(dict, "num_values", PyLong_FromUnsignedLong(num_subkeys));
> +	PyDict_SetItemString(dict, "last_change_time", PyLong_FromUnsignedLong(last_change_time_unix));
> +	PyDict_SetItemString(dict, "max_subkey_name_len", PyLong_FromUnsignedLong(num_subkeys));
> +	PyDict_SetItemString(dict, "max_value_name_len", PyLong_FromUnsignedLong(num_subkeys));
> +	PyDict_SetItemString(dict, "max_value_buf_size", PyLong_FromUnsignedLong(num_subkeys));
Likewise for the various calls here.
> 		"Delete a subkey" },
> @@ -240,6 +457,12 @@ static PyMethodDef hive_key_methods[] = {
>                    "Delete a value" },
>   	{ "set_value", py_hive_key_set_value, METH_VARARGS, "S.set_value(name, type, data) -> None\n"
>                    "Set a value" },
> +	{ "get_subkey", py_hive_get_subkey, METH_VARARGS, "S.get_subkey(name) -> HiveKey\n"
> +                 "Get a subkey object" },
> +	{ "get_value", py_hive_get_value, METH_VARARGS, "S.get_value(name) -> Dict\n"
> +                 "Get type and data of value" },
> +	{ "get_info", py_hive_get_key_info, METH_VARARGS, "S.get_info() -> Dict\n"
> +                 "Get info for the current key" },
>
The "Dict" data type in Python has a lower case "d".

Cheers,

Jelmer