Some more questions about adding my SMB Object stuff

[Jelmer Vernooij]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Richard,

On 04/07/2012 08:38 PM, Richard Sharpe wrote:
> Hi folks,
>
> In looking at implementing an SMBFile object, as suggested by a couple
> of people, I have some further questions.
>
> Here is what I currently have:
>
> diff --git a/source4/libcli/pysmb.c b/source4/libcli/pysmb.c
> index 3f2efe9..74b15bc 100644
> --- a/source4/libcli/pysmb.c
> +++ b/source4/libcli/pysmb.c
> @@ -53,6 +53,11 @@ struct smb_private_data {
> struct smbcli_tree *tree;
> };
>
> +struct smb_file_private_data {
> + struct smb_private_data *spdata;
> + int fnum;
> +};
> +
> static void dos_format(char *s)
> {
> string_replace(s, '/', '\\');
> @@ -459,6 +464,10 @@ static PyObject *py_open_file(pytalloc_Object
*self, PyObje
> spdata = self->ptr;
>
> mem_ctx = talloc_new(NULL);
> + if (!mem_ctx) {
> + PyErr_NoMemory();
> + return NULL;
> + }
>
> io.generic.level = RAW_OPEN_NTCREATEX;
> io.ntcreatex.in.root_fid.fnum = 0;
> @@ -537,7 +546,7 @@ static PyMethodDef py_smb_methods[] = {
> Set security descriptor for file." },
> { "open_file", (PyCFunction)py_open_file, METH_VARARGS,
> "open_file(path, access_mask[, share_access[, open_disposition[,
> - Open a file. Throws NTSTATUS exceptions on errors." },
> + Open a file. Throws RuntimeError exceptions for things like NT_S
> { "close_file", (PyCFunction)py_close_file, METH_VARARGS,
> "close_file(fnum) -> None\n\n \
> Close the file based on fnum."},
> @@ -613,6 +622,24 @@ static PyTypeObject PySMB = {
>
> };
>
> +/*
> + * SMBFile object
> + */
> +static PyMethodDef py_smbfile_methods[] = {
> + { "close", (PyCFunction)py_smbfile_close, METH_NOARGS,
> + "close() -> file contents as a string\n\n \
The return type here seems wrong, unless you actually expect close to
return the file contents :-)
>
> + close and destroy an smbfile object." },
This would just close the object, not actually destroy it (the python
program will still have a reference to it, and users can still try to
call methods on it).
>
> + { NULL },
> +};
> +
> +PyTypeObject PySMBFile = {
This should probably be static and const.
>
> + .tp_name = "SMBFile",
> + .tp_basicsize = sizeof(pytalloc_Object),
> + .tp_flags = Py_TPFLAGS_DEFAULT,
> + .tp_methods = py_smbfile_methods,
> + .tp_doc = "SMBFile(unc[, creds[, lp]]) -> SMBFile object\n",
> +};
> +
> void initsmb(void)
> {
> PyObject *m;
> @@ -653,4 +680,18 @@ void initsmb(void)
> ADD_FLAGS(FILE_ATTRIBUTE_NONINDEXED);
> ADD_FLAGS(FILE_ATTRIBUTE_ENCRYPTED);
> ADD_FLAGS(FILE_ATTRIBUTE_ALL_MASK);
> +
> + /* Init the other type we are creating */
> + PySMBFile.tp_base = talloc_type;

>
> +
> + if (PyType_Ready(&PySMBFile) < 0) {
> + return;
> + }
> +
> + m = Py_InitModule3("smbfile", NULL, "SMB File Object support");
> + if (m == NULL) {
> + return;
> + }
> +
> + Py_INCREF(&PySMBFile);
> }
>
> So, my questions (ignore the fact that this does not compile currently
> and there are some further fixes suggested by Jelmer):
>
> 1. I currently do not have a tp_new defined, because initially I
> expect such an object to be created via an SMB object's open_file
> (which is incorrectly named :-(). Will this be a problem?
That should be fine. The docstring for your object is wrong though,
because it suggests that there is a constructor.

>
>
> 2. I will need a close method which should Py_DECREF the SMBFile
> object to allow it to be deallocated. Is that correct?
close shouldn't actually bother with that - python will decref the
object when it goes out of scope. You probably want to guard against the
user closing the object more than once though, or trying to access it
after it has been closed.

> 3. I probably should have a tp_new defined that takes either an SMB
> object as a parameter, or a UNC string and constructs an anonymous SMB
> object. Does that sound reasonable.
It seems saner to just have open do the right thing. That way we only
have a single way to open a file, which will help with code consistency.
And it saves you time writing a constructor.

Cheers,

Jelmer
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPgNHGAAoJEACAbyvXKaRX9M8P/iwyGNi9Ynd+dx2WD8OeQhXI
5/g12C19ivZF6f4hsfL8Sb0D4pP+Ija7KFRwVKMLludPlSqH07xptCO/c6osFgDX
TPlZjeeylcSIw+EdWkqTjetg3ccZHSf+5EGcLgi6BivQOWrYelglJYKVqhANGu+n
LV2pAthTr8FVlHYOq16GfjxxIC7HDimLDzEnGHuW6EaQu23GijNMNBym6e7KwYYx
zL/WCzA3COY22Uvk2G7TjggkeaUBPzHZWPx4lazexjykuqbHmvi/slqd3plwggEl
821cWq9ufAnZZlWPEyUpaJVGUYrz5Z7GDezSxG7FiebYNkBMWz9K8BMIB/oAFswP
D8kHSJ/8HjPH8BRt+eW9AAxVN8vJcF++tTg6rndwCT4qzgv9jneRxRHO66UPrV5T
E2b7ho9k5h6TRdvA0s2JT5k0jGMe8xDje8z43qhmn29iWw+J325dGumCfOH9NfwM
0R3E8jcZ5k/T61wPlJxHgYE/qm0IpdlQp+jWiwI9wI/eDL44pL7XnUB4hLOa8T26
BLAl1yPPkmrAIQlccpAodxX9qkFmdKiHnv4cRdFQgRgz2j+5v9QU+mON/5tqI1mP
n72PVUCMKB3JqIB4mOxucd+0C9VeRvbzQQp/rhuXXYoEyAru9accILtx3sWPVZoq
CTXAE0QvDhyI/ruS5WaC
=xOS6
-----END PGP SIGNATURE-----