On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> > On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> > On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
[...]
> >> +static struct grub_scsi_test_unit_ready tur =
> >> +{
> >> + .opcode = grub_scsi_cmd_test_unit_ready,
> >> + .lun = 0,
> >> + .reserved1 = 0,
> >> + .reserved2 = 0,
> >> + .reserved3 = 0,
> >> + .control = 0,
> >> +};
> >> +
> >> +static int disks_enumerated = 0;
> >> +static struct disk_dev *disk_devs = NULL;
> >> +static struct parent_dev *parent_devs = NULL;
> >
> > I would drop all these 0/NULL initializations.
> > Compiler will do work for you. I asked about
> > that in earlier comments.
>
> I thought I changed everything I could without getting the warning
> Adrian found. I’ll try to drop these.
Thanks.
[...]
> >> +static char *
> >> +replace_escaped_commas (char *src)
> >> +{
> >> + char *iptr;
> >> +
> >> + if (src == NULL)
> >> + return NULL;
> >> + for (iptr = src; *iptr; )
> >> + {
> >> + if ((*iptr == '\\') && (*(iptr + 1) == ','))
> >> + {
> >> + *iptr++ = '_';
> >> + *iptr++ = '_';
> >> + }
> >> + iptr++;
> >> + }
> >> +
> >> + return src;
> >> +}
> >> +
> >> +static int
> >> +count_commas (const char *src)
> >> +{
> >> + int count = 0;
> >> +
> >> + for ( ; *src; src++)
> >> + if (*src == ',')
> >> + count++;
> >> +
> >> + return count;
> >> +}
> >> +
> >> +static void
> >> +escape_commas (const char *src, char *dest)
> >
> > I am confused by this play with commas. Could explain somewhere
> > where this commas are needed unescaped, escaped, replaced, etc.
> > Could not we simplify this somehow?
>
> I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",",
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
[...]
> >> +static grub_err_t
> >> +add_disk (const char *path)
> >> +{
> >> + grub_err_t ret = GRUB_ERR_NONE;
> >> + struct disk_dev *dev;
> >> + char *canon;
> >> +
> >> + canon = canonicalise_disk (path);
> >> + dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
> >> +
> >> + if ((canon != NULL) && (dev == NULL))
> >> + {
> >> + struct disk_dev *ob_device;
> >> +
> >> + ob_device = add_canon_disk (canon);
> >> +
> >> + if (ob_device == NULL)
> >> + ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
> >> + }
> >> + else if (dev)
> >> + dev->valid = 1;
> >
> > What will happen if canon == NULL?
>
> dev will always be equal to NULL in that case so nothing would happen
> other than the error being printed. But I supposed it would be better
> to have a final “else" after the "else if" and set ret = grub_error
> with GRUB_ERR_BAD_DEVICE.
Please do if you can.
[...]
> >> + grub_free (canon);
> >> + return (ret);
> >> +}
> >> +
> >> +static grub_err_t
> >> +grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
> >> + grub_size_t size, char *dest)
> >> +{
> >> + grub_err_t ret = GRUB_ERR_NONE;
> >> + struct disk_dev *dev;
> >> + unsigned long long pos;
> >> + grub_ssize_t result = 0;
> >> +
> >> + if (disk->data == NULL)
> >> + return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
> >> +
> >> + dev = (struct disk_dev *)disk->data;
> >> + pos = sector << disk->log_sector_size;
> >> + grub_ieee1275_seek (dev->ihandle, pos, &result);
> >> +
> >> + if (result < 0)
> >> + {
> >> + dev->opened = 0;
> >> + return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek
> >> block %llu",
> >> + (long long) sector);
> >> + }
> >> +
> >> + grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
> >> + &result);
> >> +
> >> + if (result != (grub_ssize_t) (size << disk->log_sector_size))
> >> + {
> >> + dev->opened = 0;
> >> + return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector
> >> 0x%llx "
> >> + "from `%s'"),
> >> + (unsigned long long) sector,
> >> + disk->name);
> >> + }
> >> + return ret;
> >> +}
> >> +
> >> +static void
> >> +grub_obdisk_close (grub_disk_t disk)
> >
> > s/grub_obdisk_close/grub_obdisk_clear/?
>
> It really is a close as far as the grub driver is concerned
> (grub_disk_dev). If you insist I’ll change it, but naming it clear
> doesn't make sense to me.
If similar functions in other drivers do just memset() or so and they
are named *close* then I am not going to insist. If it is not true then
I will ask you to do that change.
[...]
> >> +static void
> >> +iterate_devtree (const struct grub_ieee1275_devalias *alias)
> >> +{
> >> + struct grub_ieee1275_devalias child;
> >> +
> >> + if ((grub_strcmp (alias->type, "scsi-2") == 0) ||
> >> + (grub_strcmp (alias->type, "scsi-sas") == 0))
> >> + return scan_sparc_sas_disk (alias->path);
> >> +
> >> + else if (grub_strcmp (alias->type, "nvme") == 0)
> >> + return scan_nvme_disk (alias->path);
> >> +
> >> + else if (grub_strcmp (alias->type, "scsi-usb") == 0)
> >> + return scan_usb_disk (alias->path);
> >> +
> >> + else if (grub_strcmp (alias->type, "block") == 0)
> >> + {
> >> + const char **bl = block_blacklist;
> >> +
> >> + while (*bl != NULL)
> >> + {
> >> + if (grub_strstr (alias->path, *bl))
> >> + return;
> >> + bl++;
> >> + }
> >> +
> >> + add_disk (alias->path);
> >> + return;
> >> + }
> >> +
> >> + FOR_IEEE1275_DEVCHILDREN (alias->path, child)
> >> + iterate_devtree (&child);
> >> +}
> >> +
> >> +static void
> >> +unescape_devices (void)
> >
> > Why?
>
> Many SPARC disks contain a comma within the name. Code from various
> layers above this driver handle the comma differently. At times they
> will strip everything to the right of the comma. The solution I came
> up with here is self contained and will not impact generic grub2 code.
> So far it seems to work from all reports I've seen.
Great! Though I have feeling that there is still room for some
optimizations. At least we should try to do it...
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
