Re: Review request: python bindings for libiscsi

[Hans de Goede <hdegoede redhat com>]

Martin Sivak wrote:
> Hi,
>
> most of the python bindings code looks ok, but I think that you shouldn't pass native binary objects to other objects directly, but use the python way instead. Some people may want to extend your class in some way and we shouldn't be making more obstacles than necessary.

I know we already discussed this on irc. and I beleive that subclassing of 
native implemented objects should only extend those and not override any 
methods, esp. not getters and setters of native implemented atrributes.

> I'm talking about the code here:
>
>
> > static PyObject *PyIscsiNode_setAuth(PyObject *self, PyObject *args,
> > 				     PyObject *kwds)
> > {
> > 	char *kwlist[] = {"authinfo", NULL};
> > 	int err;
> > 	PyIscsiNode *node = (PyIscsiNode *)self;
> > 	PyObject *arg;
> > 	const struct libiscsi_auth_info *authinfo = NULL;
> >
> > 	if (!PyArg_ParseTupleAndKeywords(args, kwds, "O", kwlist, &arg))
> > 		return NULL;
> >
> > 	if (arg == Py_None) {
> > 		authinfo = NULL;
> > 	} else if (PyObject_IsInstance(arg, (PyObject *)
> > 				       &PyIscsiChapAuthInfo_Type)) {
> > 		PyIscsiChapAuthInfo *pyauthinfo = (PyIscsiChapAuthInfo *)arg;
>
> Especially about this line
>
> > 		authinfo = &pyauthinfo->info;
>
> In my oppinion you should create local authinfo structure and fill it with values by using something like:
>
> const struct libiscsi_auth_info authinfo;
> memset(&authinfo, 0, sizeof(struct libiscsi_auth_info));
> authinfo.method = libiscsi_auth_chap;
>
> /* get all the information we need from IscsiChapAuthInfo */
> PyObject* p_username = PyObject_GetAttrString(arg, "username");
> PyObject* p_rusername = PyObject_GetAttrString(arg, "reverse_username");
> PyObject* p_password = PyObject_GetAttrString(arg, "password");
> PyObject* p_rpassword = PyObject_GetAttrString(arg, "reverse_password");
>
> /* convert to C, so we can use libiscsi */
> if(!(p_username && p_rusername && p_password && p_rpassword)){
>   PyErr_SetString(PyExc_ValueError, "invalid authinfo type");
>   return NULL;
> }
>
> char *su, *sru, *sp, *srp;
>
> int ok = PyArg_ParseTuple(p_username, "s", &su);
> ok = ok && PyArg_ParseTuple(p_rusername, "s", &sru);
> ok = ok && PyArg_ParseTuple(p_password, "s", &sp);
> ok = ok && PyArg_ParseTuple(p_rpassword, "s", &srp);
>
> if(ok){
>   strcpy(authinfo.chap.username, su);
>   strcpy(authinfo.chap.reverse_username, sru);
>   strcpy(authinfo.chap.password, sp);
>   strcpy(authinfo.chap.reverse_password, srp);
>   err = libiscsi_node_set_auth(context, &node->node, &authinfo);
>   if (err) {
>     PyErr_SetString(PyExc_IOError,
>     		    libiscsi_get_error_string(context));
>   }
> }
> else{
>   PyErr_SetString(PyExc_ValueError, "invalid authinfo type");
> }
>
> /* decrement counters */ 
> Py_XDECREF(p_username);
> Py_XDECREF(p_rusername);
> Py_XDECREF(p_password);
> Py_XDECREF(p_rpassword);
>
>
>
> this way it would support correct subclassing of the PyIscsiChapAuthInfo.
>
> It is more code, a little bit slower, but it is the correct python way of passing arguments between classes. Were there a method returning all the auth info in one tuple/dictionary, the code would be simplier than this.

It is a lot more code, and notice how the libiscsi_auth_info struct contains a 
method field and an union, currently there is only chap support, but in the 
future there will also be radius and who knows which other methods. The way you 
propose lies madness when the "polymorphism" of the libiscsi_auth_info struct 
really starts to get used, this is why python code simply should not override 
native code, in this case there is a one on one mapping between python 
attributes and structure members, that is not always necessary the case, in 
sometimes parts of the structure may be opaque, in which case your solution 
simply will not work.

As I understand this is something which has been widely discussed in python 
land in general, and there is no consensus. As such I'm going to keep things 
doing my way, esp as when it comes to code less is more.

Regards,

Hans