"Alex Roman" <[EMAIL PROTECTED]> 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
pgpALIduaUtwO.pgp
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
