Re: CD-ROM booting staus update

[Johan Rydberg <jrydberg@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2975 bytes --]

"Alex Roman" <alex.roman@gmail.com> writes:

> I've cleaned up the code and created a patch which will enable allow
> booting from the first CD-ROM on a system. This is my first patch so I
> checked my code many times... hopefully it won't cause unwanted side
> effects, and hopefully my patch is in the correct format.

Hi Alex,

I haven't seen any reviews of this patch, so I'll give it a go.
Remember that I do not do any active GRUB2 development at the moment,
so I'll mostly comment on style related issues.


> + void EXPORT_FUNC(grub_eltorito_boot) (int drive, void *addr, int size) __attribute__ ((noreturn)); 
> +

If this line is longer than 72 characters, please try to break it.

> +  /* We can't do this for CD drives */
> +  if ( ! (drive & 0xe0) )
> +  {
> +    if (grub_biosdisk_get_diskinfo_standard (drive,
> +  					   &data->cylinders,
> +  					   &data->heads,
> +  					   &data->sectors) != 0)
> +      {
> +        grub_free (data);
> +        return grub_error (GRUB_ERR_BAD_DEVICE, "cannot get C/H/S values");
> +      }
> +  
> +    if (! total_sectors)
> +      total_sectors = data->cylinders * data->heads * data->sectors;
> +  }

I guess the outer braces are misaligned because of whitespace issues
(tab vs space)?  Otherwise you need to indent them two steps further.

>  
>    disk->total_sectors = total_sectors;
>    disk->data = data;
> @@ -167,20 +181,23 @@
>  		  unsigned segment)
>  {
>    struct grub_biosdisk_data *data = disk->data;
> -  
> +

Whitespace alterations.  No need for those.

> -      dap->block = sector;
> -
> +      dap->block =  sector;
> +      

Same here.

> +/*
> + * Decode and print a Boot Record Volume Descriptor structure
> + */

Even though the license header in each file prefixes each line with a
star, we normally don't do that for other multiline comments.

> +/*
> + * The main command function
> + */

If you ask me, no need for this comment.

> +static grub_err_t
> +grub_cmd_eltorito (struct grub_arg_list *state __attribute__ ((unused)),
> +                   int argc __attribute__ ((unused)),
> +                   char **args __attribute__ ((unused)))
> +{
> +  /* A few structure pointers that we'll need. */
> +  grub_eltorito_boot_record_vol_descr brvd;
> +  grub_eltorito_validation_entry ve;
> +  grub_eltorito_default_entry de;
> +
> +  /* Some other variables we'll need. */
> +  grub_device_t cd_dev;
> +  grub_disk_t disk;
> +  grub_err_t err;

Instead of commenting that you have declared those variables, comment
WHY.  If a comment is needed at all.  The rule of thumb is to comment
why something is done, not how.

> +struct grub_eltorito_validation_entry_tag 
> +{
> +  // Header ID, must be 0x01.
> +  grub_uint8_t    header_id;

Please do not use C++ style single-line comments.

Besides these cosmetic comments I think the overall patch looks just
fine.  Great work Alex.

~j

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]