Messages in this thread | | | From | Lukas Prediger <> | Subject | Re: [PATCH v2] drivers/cdrom: improved ioctl for media change detection | Date | Thu, 9 Sep 2021 21:05:54 +0300 |
| |
Dear Christoph, Phillip and Randy,
thanks to you all for your comments!
>>>> +static int cdrom_ioctl_timed_media_change(struct cdrom_device_info *cdi, >>>> + unsigned long arg) >>>> +{ >>>> + int ret; >>>> + struct cdrom_timed_media_change_info __user *info; >>>> + struct cdrom_timed_media_change_info tmp_info; >>>> + >>>> + if (!CDROM_CAN(CDC_MEDIA_CHANGED)) >>>> + return -ENOSYS; >>>> + >>>> + info = (struct cdrom_timed_media_change_info __user *)arg; >>>> + cd_dbg(CD_DO_IOCTL, "entering CDROM_TIMED_MEDIA_CHANGE\n"); >>>> + >>>> + ret = cdrom_ioctl_media_changed(cdi, CDSL_CURRENT); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + if (copy_from_user(&tmp_info, info, sizeof(tmp_info)) != 0) >>>> + return -EFAULT; >>>> + >>>> + tmp_info.has_changed = ((tmp_info.last_media_change - cdi->last_media_change_ms) < 0); >>> >>> 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).
>> 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.
>>> +{ >>> + __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/
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.
Best regards, Lukas
| |