Re: play.c

[Hollis Blanchard]

On Nov 29, 2005, at 4:05 PM, Vincent Pelletier wrote:
> +  status = inb (SPEAKER);
> +  outb (SPEAKER, status & ~(SPEAKER_TMR2 | SPEAKER_DATA));

You changed the style here...

> +  status = inb (SPEAKER) | SPEAKER_TMR2 | SPEAKER_DATA;
> +  outb (SPEAKER, status);

... but not here.

> +  grub_dprintf ("play","tempo = %d\n", tempo);
...
> +      grub_dprintf ("play","pitch = %d, duration = %d\n", buf.pitch,
> +                    buf.duration);

Space after the first comma, please.

> +      to = grub_get_rtc () + 120 * buf.duration / tempo;

What is 120? Should be a #define, and probably commented.

> +  file = grub_file_open (args[0]);
> +  if (! file)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "file not found");
> +
> +  if (grub_file_read (file, (void *) &tempo, sizeof(tempo)) != 
> sizeof(tempo))
> +    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +                       "file doesn't even contains a full tempo 
> record");

You must grub_file_close(file) before returning here.

Other than these minor things, looks good to me.

-Hollis