Re: [PATCH v2] treewide: rework struct gpiod_line_bulk

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Oct 27, 2020 at 10:17:15AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> 

Subject should be prefixed with [libgpiod] according to the README ;).

[snip]

> diff --git a/bindings/cxx/line_bulk.cpp b/bindings/cxx/line_bulk.cpp
> index e77baa2..e7bd20e 100644
> --- a/bindings/cxx/line_bulk.cpp
> +++ b/bindings/cxx/line_bulk.cpp
> @@ -46,6 +46,29 @@ const ::std::map<::std::bitset<32>, int, bitset_cmp> reqflag_mapping = {
>  	{ line_request::FLAG_BIAS_PULL_UP,	GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP, },
>  };
> 

A - see comment after line_bulk::event_wait()

> +struct line_bulk_iter_deleter
> +{
> +	void operator()(::gpiod_line_bulk_iter *iter)
> +	{
> +		::gpiod_line_bulk_iter_free(iter);
> +	}
> +};
> +
> +using line_bulk_iter_ptr = ::std::unique_ptr<::gpiod_line_bulk_iter,
> +					     line_bulk_iter_deleter>;
> +
> +line_bulk_iter_ptr make_line_bulk_iter(::gpiod_line_bulk *bulk)
> +{
> +	::gpiod_line_bulk_iter *iter;
> +
> +	iter = ::gpiod_line_bulk_iter_new(bulk);
> +	if (!iter)
> +		throw ::std::system_error(errno, ::std::system_category(),
> +					  "Unable to create new line bulk iterator");
> +
> +	return line_bulk_iter_ptr(iter);
> +}
> +

B - see comment after line_bulk::event_wait()

>  } /* namespace */
>  

[snip]

> @@ -263,27 +270,26 @@ line_bulk line_bulk::event_wait(const ::std::chrono::nanoseconds& timeout) const
>  {
>  	this->throw_if_empty();
>  
> -	::gpiod_line_bulk bulk, event_bulk;
> +	line_bulk_ptr ev_bulk(::gpiod_line_bulk_new(this->size()));
> +	auto chip = this->_m_bulk[0].get_chip();

Move chip into the block where it is used.

> +	auto bulk = this->to_line_bulk();
>  	::timespec ts;
>  	line_bulk ret;
>  	int rv;
>  
> -	this->to_line_bulk(::std::addressof(bulk));
> -
> -	::gpiod_line_bulk_init(::std::addressof(event_bulk));
> -
>  	ts.tv_sec = timeout.count() / 1000000000ULL;
>  	ts.tv_nsec = timeout.count() % 1000000000ULL;
>  
> -	rv = ::gpiod_line_event_wait_bulk(::std::addressof(bulk),
> -					  ::std::addressof(ts),
> -					  ::std::addressof(event_bulk));
> +	rv = ::gpiod_line_event_wait_bulk(bulk.get(), ::std::addressof(ts), ev_bulk.get());
>  	if (rv < 0) {
>  		throw ::std::system_error(errno, ::std::system_category(),
>  					  "error polling for events");
>  	} else if (rv > 0) {
> -		for (unsigned int i = 0; i < event_bulk.num_lines; i++)
> -			ret.append(line(event_bulk.lines[i], this->_m_bulk[i].get_chip()));
> +		auto iter = make_line_bulk_iter(ev_bulk.get());
> +		::gpiod_line *curr_line;
> +
> +		gpiod_line_bulk_iter_foreach_line(iter.get(), curr_line)
> +			ret.append(line(curr_line, chip));
>  	}
>  

If you replace the gpiod_line_bulk_iter_foreach_line() here with
manually looping over the bulk lines then everything from A to B above
can be dropped.

i.e. 
-               auto iter = make_line_bulk_iter(ev_bulk.get());
-               ::gpiod_line *curr_line;
+               auto chip = this->_m_bulk[0].get_chip();
+               auto num_lines = ::gpiod_line_bulk_num_lines(ev_bulk.get());

-               gpiod_line_bulk_iter_foreach_line(iter.get(), curr_line)
-                       ret.append(line(curr_line, chip));
+               for (unsigned int i = 0; i < num_lines; i++)
+                       ret.append(line(::gpiod_line_bulk_get_line(ev_bulk.get(), i), chip));

That includes the chip relocation, btw.

[snip]

>  }
> @@ -1715,9 +1751,10 @@ static PyObject *gpiod_LineBulk_event_wait(gpiod_LineBulkObject *self,
>  {
>  	static char *kwlist[] = { "sec", "nsec", NULL };
>  
> -	struct gpiod_line_bulk bulk, ev_bulk;
> -	struct gpiod_line *line, **line_ptr;
> +	struct gpiod_line_bulk *bulk, *ev_bulk;
> +	struct gpiod_line_bulk_iter *iter;
>  	gpiod_LineObject *line_obj;
> +	struct gpiod_line *line;
>  	gpiod_ChipObject *owner;
>  	long sec = 0, nsec = 0;
>  	struct timespec ts;
> @@ -1736,37 +1773,64 @@ static PyObject *gpiod_LineBulk_event_wait(gpiod_LineBulkObject *self,
>  	ts.tv_sec = sec;
>  	ts.tv_nsec = nsec;
>  
> -	gpiod_LineBulkObjToCLineBulk(self, &bulk);
> +	bulk = gpiod_LineBulkObjToCLineBulk(self);
> +	if (!bulk)
> +		return NULL;
> +
> +	ev_bulk = gpiod_line_bulk_new(self->num_lines);
> +	if (!ev_bulk) {
> +		gpiod_line_bulk_free(bulk);
> +		return NULL;
> +	}
>  
>  	Py_BEGIN_ALLOW_THREADS;
> -	rv = gpiod_line_event_wait_bulk(&bulk, &ts, &ev_bulk);
> +	rv = gpiod_line_event_wait_bulk(bulk, &ts, ev_bulk);
> +	gpiod_line_bulk_free(bulk);
>  	Py_END_ALLOW_THREADS;
> -	if (rv < 0)
> +	if (rv < 0) {
> +		gpiod_line_bulk_free(ev_bulk);
>  		return PyErr_SetFromErrno(PyExc_OSError);
> -	else if (rv == 0)
> +	} else if (rv == 0) {
> +		gpiod_line_bulk_free(ev_bulk);
>  		Py_RETURN_NONE;
> +	}
>  
> -	ret = PyList_New(gpiod_line_bulk_num_lines(&ev_bulk));
> -	if (!ret)
> +	ret = PyList_New(gpiod_line_bulk_num_lines(ev_bulk));
> +	if (!ret) {
> +		gpiod_line_bulk_free(ev_bulk);
>  		return NULL;
> +	}
>  
>  	owner = ((gpiod_LineObject *)(self->lines[0]))->owner;
>  
> +	iter = gpiod_line_bulk_iter_new(ev_bulk);
> +	if (!iter) {
> +		gpiod_line_bulk_free(ev_bulk);
> +		return PyErr_SetFromErrno(PyExc_OSError);
> +	}
> +
>  	i = 0;
> -	gpiod_line_bulk_foreach_line(&ev_bulk, line, line_ptr) {
> +	gpiod_line_bulk_iter_foreach_line(iter, line) {
>  		line_obj = gpiod_MakeLineObject(owner, line);
>  		if (!line_obj) {
> +			gpiod_line_bulk_iter_free(iter);
> +			gpiod_line_bulk_free(ev_bulk);
>  			Py_DECREF(ret);
>  			return NULL;
>  		}
>  
>  		rv = PyList_SetItem(ret, i++, (PyObject *)line_obj);
>  		if (rv < 0) {
> +			gpiod_line_bulk_iter_free(iter);
> +			gpiod_line_bulk_free(ev_bulk);
>  			Py_DECREF(ret);
>  			return NULL;
>  		}
>  	}
>  

This function can be simplified by looping over the bulk manually rather
than using the line_bulk_iter.

> +	gpiod_line_bulk_iter_free(iter);
> +	gpiod_line_bulk_free(ev_bulk);
> +
>  	return ret;
>  }
>  
> @@ -2241,41 +2305,59 @@ PyDoc_STRVAR(gpiod_Chip_get_all_lines_doc,
>  static gpiod_LineBulkObject *
>  gpiod_Chip_get_all_lines(gpiod_ChipObject *self, PyObject *Py_UNUSED(ignored))
>  {
> +	struct gpiod_line_bulk_iter *iter;
>  	gpiod_LineBulkObject *bulk_obj;
> -	struct gpiod_line_bulk bulk;
> +	struct gpiod_line_bulk *bulk;
>  	gpiod_LineObject *line_obj;
>  	struct gpiod_line *line;
> -	unsigned int offset;
> +	unsigned int index = 0;
>  	PyObject *list;
>  	int rv;
>  
>  	if (gpiod_ChipIsClosed(self))
>  		return NULL;
>  
> -	rv = gpiod_chip_get_all_lines(self->chip, &bulk);
> -	if (rv)
> +	bulk = gpiod_chip_get_all_lines(self->chip);
> +	if (!bulk)
>  		return (gpiod_LineBulkObject *)PyErr_SetFromErrno(
>  							PyExc_OSError);
>  
> -	list = PyList_New(gpiod_line_bulk_num_lines(&bulk));
> -	if (!list)
> +	list = PyList_New(gpiod_line_bulk_num_lines(bulk));
> +	if (!list) {
> +		gpiod_line_bulk_free(bulk);
>  		return NULL;
> +	}
>  
> -	gpiod_line_bulk_foreach_line_off(&bulk, line, offset) {
> +	iter = gpiod_line_bulk_iter_new(bulk);
> +	if (!iter) {
> +		gpiod_line_bulk_free(bulk);
> +		Py_DECREF(list);
> +		return (gpiod_LineBulkObject *)PyErr_SetFromErrno(
> +							PyExc_OSError);
> +	}
> +
> +	gpiod_line_bulk_iter_foreach_line(iter, line) {
>  		line_obj = gpiod_MakeLineObject(self, line);
>  		if (!line_obj) {
> +			gpiod_line_bulk_iter_free(iter);
> +			gpiod_line_bulk_free(bulk);
>  			Py_DECREF(list);
>  			return NULL;
>  		}
>

Again, NOT using the line_bulk_iter here, and manually looping instead, is
actually simpler and cleaner.

Cheers,
Kent.