Sorry for late reply but I was busy with other stuff.
On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
> > 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 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
Do you mean from the command line? If yes could you give an example with
proper escaping?
> the driver talks to the actual hardware these devices can not have the
> escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
> > 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.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
e.g. name and name_esc. And I would do this:
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
What is replace_escaped_commas()? Why do we need that function?
[...]
> >>>> + 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.
So, lets leave it as is.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
