> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
>
> 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 ",”,
AFAIK all layers above expect it:
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when the
driver talks to the actual hardware these devices can not have the escaped
names.
> how often this conversions must be done, why you have chosen that
> solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they are
enumerated. All disks are cached within this driver so it should not happen
often. That is why I store both versions so I don’t have to go back and forth.
Look at the current driver ofdisk. It has a function called compute_dev_path
which does this conversion on every single open (grub_ofdisk_open). That does
not happen with this new driver. IMHO this is a much more optimized solution
than the current driver.
>
> [...]
>
>>>> +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.
I’ll take care of this.
>
> [...]
>
>>>> + 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.
From what I can see _close seems like the standard name here. Some drivers
such as efidisk just do a grub_dprintf and nothing more within its close.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
