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

[Phillip Potter <phil@philpotter.co.uk>]

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.

> >>> 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.

> >> 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 :-)

> >>> +{
> >>> +	__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/
> 

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