Re: [PATCH v2] ieee1275: obdisk driver

[Eric Snowberg]

> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <address@hidden> wrote:
> 
> 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 <address@hidden> wrote:
>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <address@hidden> 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?

This goes much further than the command line.  For example, it is built right 
into the disk driver.  Look at grub-core/kern/disk.c: 544 

/* Return the location of the first ',', if any, which is not
   escaped by a '\'.  */
static const char *
find_part_sep (const char *name)
{
  const char *p = name;
  char c;

  while ((c = *p++) != '\0')
    {
      if (c == '\\' && *p == ',')
        p++;
      else if (c == ',')
        return p - 1;
    }
  return NULL;
}

When the obdisk driver discovers a disk, it must put the disk name in the 
proper format.  Otherwise when grub_disk_open takes place later on, the wrong 
disk name will eventually get sent back to the obdisk driver.


> 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.

Ok

> 
>>> 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.

Ok

> 
> What is replace_escaped_commas()? Why do we need that function?

It is a convenience function for the end-user, so they can access a disk 
without having to understand all this escaping when they want to use one thru 
the shell.