Code review comment for lp:~bladernr/checkbox/945178-make-auto-cd-read-test

Revision history for this message
Jeff Lane  (bladernr) wrote :

> I know this patch is to the job file, not the script, optical_read_test.
> But I take a look at optical_read_test, found few issue should be taken care
> in the script.

No worries, those are issues that need to be addressed. See below:

> Few exceptions here:
> 1)
> command: optical_read_test /dev/cdrom (but /dev/cdrom is not existed
> actually).
> command passed and return 0.

Yeah, that was a flaw in the original script design. Now if DEVICE passed in doesn't exist (/lib/udev/cdrom_id will error if DEVICE does not exist or is inaccessible), the test will spit out an error message and exit with a FAIL.

> 2)
> command: optical_read_test
> command passed and return 0.

Heh, yeah, this was broke too. Changed it to use argparse and if you don't specify at least one DEVICE, the script will throw a parser.error and exit with a FAIL.

> 3)
> Not sure the device name /dev/cdrom is always reliable. I would suggest we use
> the device name from "wodim --devices"
> Sample output is like:
> wodim: Overview of accessible drives (1 found) :
> -------------------------------------------------------------------------
> 0 dev='/dev/sg1' rwrw-- : 'Slimtype' 'DVD A DS8A4S'
> -------------------------------------------------------------------------
>
> We can take the dev='xxxx' part.

At a minimum, /dev/cdrom should always be present on a system with an optical drive and that should be a symlink to /dev/sr[0-9]. If a device has an optical drive and is missing at least /dev/cdrom, there's a problem. For example:

$ ls -l /dev/cd*
lrwxrwxrwx 1 root root 3 Mar 16 19:20 /dev/cdrom -> sr0
lrwxrwxrwx 1 root root 3 Mar 16 19:20 /dev/cdrw -> sr0

You CAN access the device via the sr device, but I don't think you can access it via the sg:

$ /lib/udev/cdrom_id /dev/sr0
ID_CDROM=1
ID_CDROM_CD=1
ID_CDROM_CD_R=1
ID_CDROM_CD_RW=1
ID_CDROM_DVD=1
ID_CDROM_MRW=1
ID_CDROM_MRW_W=1
$ /lib/udev/cdrom_id /dev/sg0
unable to open '/dev/sg0'

> 4) optical_read_test can handle multi cd-rom test.(yes,I don't understand why
> so many cd-roms installed on server too), But the job files should be changed
> too. We may have job name like: optical/read_*

We used to have this, actually, with the drives. However, it's pretty much a guarantee that we will never certify a machine that has more than 1 optical drive. In fact, outside of special order systems (and even that's questionable) you'll have difficulty even finding systems for sale with more than one optical drive, if any.

> So now, I would say 1) 2) should be fixed. 3) 4) can be taken care later.

1 and 2 are fixed. I don't think 3 is really an issue, as I said, if a system with an optical drive doesn't have a minimum of the common /dev/cdrom symlink there's a kernel or driver issue that needs to be exposed and investigated. As for 4, we did have that, removed it because it's highly unlikely to ever need that, but we can always revisit that later and change it back. The script will certainly handle multiple devices, so as you noted, the only change would be to the jobs and whitelists.

review: Needs Resubmitting

« Back to merge proposal