On 11/25/21 21:12, Gerald Schaefer wrote:

On Thu, 25 Nov 2021 17:13:10 +0100
Gerald Schaefer [off-list ref] wrote:

quoted

On Tue, 23 Nov 2021 15:19:49 +0100
Vlastimil Babka [off-list ref] wrote:

quoted

On 11/22/21 21:33, Gerald Schaefer wrote:

quoted

On Mon, 22 Nov 2021 21:14:00 +0100
Gerald Schaefer [off-list ref] wrote:

[...]

quoted

Thanks. While testing this properly, yet another bug showed up. The idx
in op->show remains 0 in all iterations, so I always see the same line
printed t->count times (or infinitely, ATM). Not sure if this only shows
on s390 due to endianness, but the reason is this:

  unsigned int idx = *(unsigned int *)v;

Uh, good catch. I was actually looking suspiciously at how we cast signed to
unsigned, but didn't occur to me that shortening together with endiannes is
the problem.

quoted

quoted

IIUC, void *v is always the same as loff_t *ppos, and therefore idx also
should be *ppos. De-referencing the loff_t * with an unsigned int * only
gives the upper 32 bit half of the 64 bit value, which remains 0.

This would be fixed e.g. with

  unsigned int idx = (unsigned int) *(loff_t *) v;

With all this experience I'm now inclined to rather follow more the example
in Documentation/filesystems/seq_file.rst and don't pass around the pointer
that we got as ppos in slab_debugfs_start(), and that seq_file.c points to
m->index.

In that example an own value is kmalloced:

loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);

while we could just make this a field of loc_track?

Yes, following the example sounds good, and it would also make proper use
of *v in op->next, which might make the code more readable. It also looks
like it already does exactly what is needed here, i.e. have a simple
iterator that just counts the lines.

I don't think the iterator needs to be saved in loc_track. IIUC, it is
already passed around like in the example, and can then be simply compared
to t->count, similar to the existing code.

Saving it the loc_track doesn't preclude using the pointer that's being
passed around. It however avoids the extra kmalloc and turns out it
should also solve the return NULL from op->next() issue you describe below?

quoted

This is what I'm currently testing, and it seems to work fine. Will send
a new patch, if there are no objections:

Oh well, I have one objection, returning NULL from op->next will be
passed to op->stop, and then it will not free the allocated value.

The example is elegantly avoiding this, by not returning NULL anywhere,
and also not stopping. Sigh.

Maybe not return NULL in op->next, but only from op->start, and only
when no allocation was made or it was freed already? Or free it only/
already in op->next, when returning NULL?

From these two probably the "free in op->next". But still inclined to
store it in loc_track.
Why does the API need to be so awkward...