Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection

On Thu, Sep 09, 2021 at 09:05:54PM +0300, Lukas Prediger wrote:

Dear Christoph, Phillip and Randy,

thanks to you all for your comments!

Dear Lukas,

You're welcome, thank you for the code.

quoted

quoted

quoted

Overly long line here, but more importantly this is much cleaner with
a good old if:


	if (tmp_info.last_media_change - cdi->last_media_change_ms) < 0)
		tmp_info.has_changed = 1;

Whilst I don't disagree this is technically cleaner, the existing style
certainly read well to me. 

The if would additionally require to explicitly initialise .has_changed to
zero for the else case, so I favored the single assignment that covers
all cases. I don't have a strong opinion on this, though, so if the if variant
is generally favored, I can change this. (And I will definitely fix the overlength).

Yes true, but I guess your existing style is harder to split across
lines in a clean way. As mentioned, I didn't mind the original code, but
the line length is a fair point. Your call on this one - those with far
more experience than me would probably argue the if/else form though.

quoted

quoted

In terms of line length, checkpatch doesn't
complain about it, so I guess you mean purely from a visual perspective?

Documentation/process/coding-style.rst says:

   The preferred limit on the length of a single line is 80 columns.

checkpatch only checks lines > 100 columns since that is OK in a few
cases, like a long quoted string.

So try to limit line lengths to 80 columns unless there is some
other reason not to do that.

I wasn't aware that checkpatch.pl does not complain if I exceed the 80 cols,
have fixed those now for an upcoming resubmission.

Same, guilty as charged on this one - live and learn I guess :-)

quoted

quoted

quoted

+{
+	__s64	last_media_change;	/* Timestamp of the last detected media
+					 * change in ms. May be set by caller, updated
+					 * upon successful return of ioctl.
+					 */
+	__u64	has_changed;		/* Set to 1 by ioctl if last detected media

More overly long lines.  Also why is has_changed a u64 if it is used as
a boolean flag?

As this is not a packed struct, would not a smaller value still take up
the same space?

Might as well be explicit about it and also make it obvious that there
is some space available for other fields.

I had this as a __u8 in the first submission but Jens asked me to change it.
From his feedback on this:

"The struct layout should be modified such that there are no holes or
padding in it. Probably just make the has_changed a flags thing, and
make it u64 as well. Then you can define bit 0 to be HAS_CHANGED, and
that leaves you room to add more flags in the future."
https://lore.kernel.org/lkml/6d6c533d-465e-33ee-5801-cb7ea84924a8@kernel.dk/ (local)

Yeah, maybe just a bit more in the comment to emphasize the room for
extra bits in has_changed? I agree it looks fine like this to me though
given the lack of struck packing anyway.

I changed it to __u64 to address this. We could think about turning it
back to a __u8 (or bool) and add some explicit padding members
(a __u8 reserved[3]?), but honestly I don't see much real benefit in that
compared to how it is now.

I agree with you on this personally, I think it's fine.

Regards,
Phil