| Message ID | da032629db4a770a5f98ff400b91b44873cbdf46.1571834862.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix cdrom autoclose. | expand |
> static > -int open_for_data(struct cdrom_device_info *cdi) > +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks) Please fix the coding style. static never should be on a line of its own.. > } else { > cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n"); > - ret=-ENOMEDIUM; > - goto clean_up_and_return; > + return -ENOMEDIUM; Can you revert the polarity of the if opening the block before and return early for the -ENOMEDIUM case to save on leven of indentation? > if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) { If you touch the whole area please remove the inner braces and add the proper spaces around the second ==. > +static > +int open_for_data(struct cdrom_device_info *cdi) Same issue with the static here.
On Wed, Oct 23, 2019 at 07:19:58PM -0700, Christoph Hellwig wrote: > > static > > -int open_for_data(struct cdrom_device_info *cdi) > > +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks) > > Please fix the coding style. static never should be on a line of its > own.. That's fine. > > > } else { > > cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n"); > > - ret=-ENOMEDIUM; > > - goto clean_up_and_return; > > + return -ENOMEDIUM; > > Can you revert the polarity of the if opening the block before and > return early for the -ENOMEDIUM case to save on leven of indentation? Then I will get complaints I do unrelated changes and it's hard to review. The code gets removed later anyway. Thanks Michal
On Wed, Oct 23, 2019 at 07:19:58PM -0700, Christoph Hellwig wrote: > > static > > -int open_for_data(struct cdrom_device_info *cdi) > > +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks) > > Please fix the coding style. static never should be on a line of its > own.. It's OK to have the static on a line by itself; it's having 'static int' on a line by itself that Linus gets unhappy about because he can't use grep to see the return type. But there's no need for it to be on a line by itself here, it all fits fine in 80 columns.
On Thu, Oct 24, 2019 at 06:23:14AM -0700, Matthew Wilcox wrote: > On Wed, Oct 23, 2019 at 07:19:58PM -0700, Christoph Hellwig wrote: > > > static > > > -int open_for_data(struct cdrom_device_info *cdi) > > > +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks) > > > > Please fix the coding style. static never should be on a line of its > > own.. > > It's OK to have the static on a line by itself; it's having 'static int' > on a line by itself that Linus gets unhappy about because he can't use > grep to see the return type. Sorry, but independent of any preference just looking at the codebases proves you wrong. All on one line is the most common style, but not by much, followed by static + type. Just static is just in a few crazy
On Thu, Oct 24, 2019 at 10:50:14AM +0200, Michal Suchánek wrote: > Then I will get complaints I do unrelated changes and it's hard to > review. The code gets removed later anyway. If you refactor you you pretty much have a card blanche for the refactored code and the direct surroundings.
On Thu, Oct 24, 2019 at 07:39:08PM -0700, Christoph Hellwig wrote: > On Thu, Oct 24, 2019 at 10:50:14AM +0200, Michal Suchánek wrote: > > Then I will get complaints I do unrelated changes and it's hard to > > review. The code gets removed later anyway. > > If you refactor you you pretty much have a card blanche for the > refactored code and the direct surroundings. This is different from what other reviewers say: https://lore.kernel.org/lkml/1517245320.2687.14.camel@wdc.com/ Either way, this code is removed in a later patch so this discussion is moot. It makes sense to have a bisection point here in case something goes wrong but it is pointless to argue about the code structure inherited from the previous revision. Thanks Michal
On Fri, 25 Oct 2019, Michal Such?nek wrote: > On Thu, Oct 24, 2019 at 07:39:08PM -0700, Christoph Hellwig wrote: > > On Thu, Oct 24, 2019 at 10:50:14AM +0200, Michal Such?nek wrote: > > > Then I will get complaints I do unrelated changes and it's hard to > > > review. The code gets removed later anyway. > > > > If you refactor you you pretty much have a card blanche for the > > refactored code and the direct surroundings. > > This is different from what other reviewers say: > > https://lore.kernel.org/lkml/1517245320.2687.14.camel@wdc.com/ > I don't see any inconsistency there. Both reviews are valuable. In general, different reviewers may give contradictory advice. Reviewers probably even contradict themselves eventually. Yet it rarely happens that the same patch gets contradictory reviews. If it did, you might well complain. > Either way, this code is removed in a later patch so this discussion is > moot. > > It makes sense to have a bisection point here in case something > goes wrong but it is pointless to argue about the code structure > inherited from the previous revision. A patch may refactor some code only to have the next patch remove that code. This doesn't generally mean that the former patch is redundant. The latter patch may end up committed and subsequently reverted. The latter patch may become easier to review because of the former. The former patch may be eligible for -stable. The former patch may be the result of an automatic process. And so on. I don't know what Christoph had in mind here but he's usually right, so it's worth asking.
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index 2ad6b73482fe..f504ca70f93f 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -1046,12 +1046,12 @@ static void cdrom_count_tracks(struct cdrom_device_info *cdi, tracktype *tracks) } static -int open_for_data(struct cdrom_device_info *cdi) +int open_for_common(struct cdrom_device_info *cdi, tracktype *tracks) { int ret; const struct cdrom_device_ops *cdo = cdi->ops; - tracktype tracks; - cd_dbg(CD_OPEN, "entering open_for_data\n"); + + cd_dbg(CD_OPEN, "entering open_for_common\n"); /* Check if the driver can report drive status. If it can, we can do clever things. If it can't, well, we at least tried! */ if (cdo->drive_status != NULL) { @@ -1071,37 +1071,45 @@ int open_for_data(struct cdrom_device_info *cdi) couldn't close the tray. We only care that there is no disc in the drive, since that is the _REAL_ problem here.*/ - ret=-ENOMEDIUM; - goto clean_up_and_return; + return -ENOMEDIUM; } } else { cd_dbg(CD_OPEN, "bummer. this drive can't close the tray.\n"); - ret=-ENOMEDIUM; - goto clean_up_and_return; + return -ENOMEDIUM; } /* Ok, the door should be closed now.. Check again */ ret = cdo->drive_status(cdi, CDSL_CURRENT); if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) { cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n"); cd_dbg(CD_OPEN, "tray might not contain a medium\n"); - ret=-ENOMEDIUM; - goto clean_up_and_return; + return -ENOMEDIUM; } cd_dbg(CD_OPEN, "the tray is now closed\n"); } - /* the door should be closed now, check for the disc */ - ret = cdo->drive_status(cdi, CDSL_CURRENT); - if (ret!=CDS_DISC_OK) { - ret = -ENOMEDIUM; - goto clean_up_and_return; - } + if (ret != CDS_DISC_OK) + return -ENOMEDIUM; } - cdrom_count_tracks(cdi, &tracks); - if (tracks.error == CDS_NO_DISC) { + cdrom_count_tracks(cdi, tracks); + if (tracks->error == CDS_NO_DISC) { cd_dbg(CD_OPEN, "bummer. no disc.\n"); - ret=-ENOMEDIUM; - goto clean_up_and_return; + return -ENOMEDIUM; } + + return 0; +} + +static +int open_for_data(struct cdrom_device_info *cdi) +{ + int ret; + const struct cdrom_device_ops *cdo = cdi->ops; + tracktype tracks; + + cd_dbg(CD_OPEN, "entering open_for_data\n"); + ret = open_for_common(cdi, &tracks); + if (ret) + goto clean_up_and_return; + /* CD-Players which don't use O_NONBLOCK, workman * for example, need bit CDO_CHECK_TYPE cleared! */ if (tracks.data==0) { @@ -1208,53 +1216,17 @@ int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev, /* This code is similar to that in open_for_data. The routine is called whenever an audio play operation is requested. */ -static int check_for_audio_disc(struct cdrom_device_info *cdi, - const struct cdrom_device_ops *cdo) +static int check_for_audio_disc(struct cdrom_device_info *cdi) { int ret; tracktype tracks; cd_dbg(CD_OPEN, "entering check_for_audio_disc\n"); if (!(cdi->options & CDO_CHECK_TYPE)) return 0; - if (cdo->drive_status != NULL) { - ret = cdo->drive_status(cdi, CDSL_CURRENT); - cd_dbg(CD_OPEN, "drive_status=%d\n", ret); - if (ret == CDS_TRAY_OPEN) { - cd_dbg(CD_OPEN, "the tray is open...\n"); - /* can/may i close it? */ - if (CDROM_CAN(CDC_CLOSE_TRAY) && - cdi->options & CDO_AUTO_CLOSE) { - cd_dbg(CD_OPEN, "trying to close the tray\n"); - ret=cdo->tray_move(cdi,0); - if (ret) { - cd_dbg(CD_OPEN, "bummer. tried to close tray but failed.\n"); - /* Ignore the error from the low - level driver. We don't care why it - couldn't close the tray. We only care - that there is no disc in the drive, - since that is the _REAL_ problem here.*/ - return -ENOMEDIUM; - } - } else { - cd_dbg(CD_OPEN, "bummer. this driver can't close the tray.\n"); - return -ENOMEDIUM; - } - /* Ok, the door should be closed now.. Check again */ - ret = cdo->drive_status(cdi, CDSL_CURRENT); - if ((ret == CDS_NO_DISC) || (ret==CDS_TRAY_OPEN)) { - cd_dbg(CD_OPEN, "bummer. the tray is still not closed.\n"); - return -ENOMEDIUM; - } - if (ret!=CDS_DISC_OK) { - cd_dbg(CD_OPEN, "bummer. disc isn't ready.\n"); - return -EIO; - } - cd_dbg(CD_OPEN, "the tray is now closed\n"); - } - } - cdrom_count_tracks(cdi, &tracks); - if (tracks.error) - return(tracks.error); + + ret = open_for_common(cdi, &tracks); + if (ret) + return ret; if (tracks.audio==0) return -EMEDIUMTYPE; @@ -2725,7 +2697,7 @@ static int cdrom_ioctl_play_trkind(struct cdrom_device_info *cdi, if (copy_from_user(&ti, argp, sizeof(ti))) return -EFAULT; - ret = check_for_audio_disc(cdi, cdi->ops); + ret = check_for_audio_disc(cdi); if (ret) return ret; return cdi->ops->audio_ioctl(cdi, CDROMPLAYTRKIND, &ti); @@ -2773,7 +2745,7 @@ static int cdrom_ioctl_audioctl(struct cdrom_device_info *cdi, if (!CDROM_CAN(CDC_PLAY_AUDIO)) return -ENOSYS; - ret = check_for_audio_disc(cdi, cdi->ops); + ret = check_for_audio_disc(cdi); if (ret) return ret; return cdi->ops->audio_ioctl(cdi, cmd, NULL);
The open_for_audio and open_for_data copies are bitrotten in different ways already and will need to update the autoclose logic in both. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- drivers/cdrom/cdrom.c | 96 +++++++++++++++---------------------------- 1 file changed, 34 insertions(+), 62 deletions(-)