grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] ieee1275: obdisk driver


From: Eric Snowberg
Subject: Re: [PATCH v2] ieee1275: obdisk driver
Date: Thu, 30 Aug 2018 09:28:52 -0600

> On Aug 30, 2018, at 8:06 AM, Daniel Kiper <address@hidden> wrote:
> 
> On Tue, Jul 17, 2018 at 09:59:33AM -0600, Eric Snowberg wrote:
>>> On Jul 17, 2018, at 7:38 AM, Daniel Kiper <address@hidden> wrote:
>>> On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
>>>>> 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
>>> 
>>> This is the last line of the file. So, I am not sure what exactly you mean.
>>> 
>>>> /* 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;
>>>> }
>>> 
>>> OK, this one.
>>> 
>>>> 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.
>>> 
>>> Then we need proper escaping. And AIUI your driver does that.
>>> 
>>>>> 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.
>>> 
>>> I think that this will introduce more confusion. I would like that
>>> escaping of drive paths should be properly explained in GRUB docs.
>>> Including why it is needed. And replace_escaped_commas() should be
>>> dropped.
>> 
>> The confusion seems to be around what needs to be escaped and what
>> doesn’t, as can be seen from the discussion within this email. This
>> change makes it convenient for the end-user, since they will not need
>> to understand any of this.
>> 
>> When function grub_obdisk_iterate (defined as .iterate within
>> grub_disk_dev) gets called, it returns the disks the driver controls.
>> As explained within the description of this patch, a single IEEE1275
>> disk can have over 6 names.  When the .iterate function is called,
>> only a single drive can be returned.  If the disk that is to be
>> returned contains \\, within the name, they are replaced with __.
>> Then for example, the end-user will see something like this following
>> a ls:
>> 
>> grub> ls
>> (ieee1275//address@hidden/address@hidden/address@hidden/address@hidden/address@hidden/address@hidden)
>>  (ieee1275//address@hidden/pc
>> address@hidden/address@hidden/address@hidden/address@hidden/address@hidden,gpt1)
>>  (ieee1275//address@hidden/address@hidden/address@hidden
>> /address@hidden) 
>> (ieee1275//address@hidden/address@hidden/address@hidden/address@hidden,gpt3) 
>> (ieee1275//pc
>> address@hidden/address@hidden/address@hidden/address@hidden,gpt2) 
>> (ieee1275//address@hidden/address@hidden/address@hidden/
>> address@hidden,gpt1)
>> 
>> The end-user can now type the disk name exactly as it is returned on
>> the screen.  Or they can escape any of the real disk names properly
>> and the driver will understand it is the same disk.  Do you really
>> want this removed?
> 
> After some thinking it seems to me that we should remove this "__"
> feature. It introduces another specific escaping form. And IMO this will
> make more confusion then it is worth. And what if disk path contains
> "__”? Yeah, I know probably it is not the case right now but we should
> be prepared…
> Though I am not against displaying properly escaped
> disks and partitions paths, e.g.:
> 
> (ieee1275//address@hidden/address@hidden/address@hidden/address@hidden/address@hidden/address@hidden,0)
> 
> or
> 
> (ieee1275//address@hidden/address@hidden/address@hidden/address@hidden/address@hidden/address@hidden,0,gpt1)
> 
> etc.

I don’t believe you have escaped the name properly above.  Unless you are 
suggesting substituting ‘\\’ with “\\” before the item is displayed.  If that 
is acceptable, would you accept this change instead?

+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;
+}


> 
> Additionally, I think that you should update GRUB2 docs in relevant places
> and add an info saying that disk paths containing "," should be properly
> escaped.
> 

It is already documented here: 
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax





reply via email to

[Prev in Thread] Current Thread [Next in Thread]