On Mon, Sep 06, 2021 at 02:11:31PM -0600, Jens Axboe wrote:
> On 8/29/21 8:37 AM, Lukas Prediger wrote:
> > The current implementation of the CDROM_MEDIA_CHANGED ioctl relies on
> > global state, meaning that only one process can detect a disc change
> > while the ioctl call will return 0 for other calling processes afterwards
> > (see bug 213267 ).
> >
> > This introduces a new cdrom ioctl, CDROM_TIMED_MEDIA_CHANGE, that
> > works by maintaining a timestamp of the last detected disc change instead
> > of a boolean flag: Processes calling this ioctl command can provide
> > a timestamp of the last disc change known to them and receive
> > an indication whether the disc was changed since then and the updated
> > timestamp.
> >
> > I considered fixing the buggy behavior in the original
> > CDROM_MEDIA_CHANGED ioctl but that would require maintaining state
> > for each calling process in the kernel, which seems like a worse
> > solution than introducing this new ioctl.
>
> This looks pretty good to me now. Adding Phillip to the CC, he's the new
> CDROM maintainer. Leaving the rest of the message below intact because
> of that.
>
> >
...
> >
Dear Lukas,
Thank you for the patch, much appreciated and looks great. One very
minor thing though has jumped out at me after running checkpatch though:
> > --- a/include/uapi/linux/cdrom.h
> > +++ b/include/uapi/linux/cdrom.h
> > @@ -147,6 +147,8 @@
> > #define CDROM_NEXT_WRITABLE 0x5394 /* get next writable block */
> > #define CDROM_LAST_WRITTEN 0x5395 /* get last block written on disc */
> >
> > +#define CDROM_TIMED_MEDIA_CHANGE 0x5396 /* get the timestamp of the last media change */
> > +
> > /*******************************************************
> > * CDROM IOCTL structures
> > *******************************************************/
> > @@ -295,6 +297,19 @@ struct cdrom_generic_command
> > };
> > };
> >
> > +/* This struct is used by CDROM_TIMED_MEDIA_CHANGE */
> > +struct cdrom_timed_media_change_info
> > +{
This opening brace should be on the same line as the struct definition
as per current style rules.
I also got a checkpatch warning about ENOSYS being used as an error
value, but I can see this usage is standard in the driver and not a
problem so no issue with that.
I will review and test properly after work tomorrow (being new to the
role I'd like to make sure I'm taking the proper time), but I have no
doubt it will work fine. Assuming it does I will be happy to accept the
patch with the above brace change. Thanks again.
> > + __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
> > + * change was more recent than
> > + * last_media_change set by caller.
> > + */
> > +};
> > +
> > /*
> > * A CD-ROM physical sector size is 2048, 2052, 2056, 2324, 2332, 2336,
> > * 2340, or 2352 bytes long.
> >
Regards,
Phil
