Re: [PATCH] GPT Shell Application/Library

[Laszlo Ersek <lersek@redhat.com>]

On 10/18/16 20:03, Vladimir Olovyannikov wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: October-18-16 10:24 AM
>> To: Vladimir Olovyannikov; Shah, Tapan; Carsey, Jaben; Michael Zimmermann
>> Cc: Ni, Ruiyu; Arshi, Shala; edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH] GPT Shell Application/Library
>>
>> On 10/18/16 18:58, Vladimir Olovyannikov wrote:
>>> Thank you all for comments,
>>>
>>> So to summarize the discussion:
>>>
>>> 1. I will create a Shell library which would perform all GPT operations.
>>>      Part of PartitionDxe will also be in that library so PartitionDxe
>>> will be using it.
>>>      The gpt Shell tool will also be using it.
>>
>> I think you might want to place the library class header file, and the
>> library
>> instance, under MdeModulePkg. It's okay for the shell application (or
>> shell
>> libraries) to depend on library classes / library instances from
>> MdeModulePkg, but -- I think -- it's not okay for a driver in
>> MdeModulePkg,
>> such as PartitionDxe, to depend on a class + instance from under ShellPkg.
> Laszlo,
> I think PartitionDxe will depend on the GPT shared library (which will have
> nothing to do to the ShellPkg),
> as well as gpt tool (Shell library/application) will depend on that shared
> library as well to get information regarding partitions and
> to perform GPT management.

I understood that, yes. My point was the location of the new common
library (class header and instance both). You wrote

"I will create a Shell library ..."

which made me think you wanted to place the files under:

  ShellPkg/Include/Library/...
  ShellPkg/Library/...

which is not right in this instance. I suggested to place those files under

  MdeModulePkg/Include/Library/...
  MdeModulePkg/Library/...

Thanks!
Laszlo

> 
> Thank you,
> Vladimir
>>
>> Thanks!
>> Laszlo
>>
>>> 2.  Refactor the parameters of the gpt utility to make it similar to
>>> other existing Shell commands.
>>>       BTW Is there any document describing Shell utility parameters'
>>> standards?
>>>
>>> Please let me know if you have other suggestions.
>>>
>>> Thank you,
>>> Vladimir
>>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Shah, Tapan
>>> Sent: October-18-16 6:59 AM
>>> To: Carsey, Jaben; Vladimir Olovyannikov; Michael Zimmermann
>>> Cc: Ni, Ruiyu; Arshi, Shala; edk2-devel@lists.01.org; Laszlo Ersek
>>> Subject: Re: [edk2] [PATCH] GPT Shell Application/Library
>>>
>>> Thanks for the contribution Vladimir!
>>>
>>> Few comments:
>>> 1. It's better to refactor the code now before commit and move GPT
>>> related code outside ShellPkg and create a shared library.
>>> 2. CLI parameters of this utility are too complex and need to be
>>> refactored to make it similar to other existing Shell commands.
>>>
>>> Thanks,
>>> Tapan
>>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Carsey, Jaben
>>> Sent: Monday, October 17, 2016 12:56 PM
>>> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
>>> Michael Zimmermann <sigmaepsilon92@gmail.com>
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Arshi, Shala
>>> <shala.arshi@intel.com>; edk2-devel@lists.01.org
>>> <edk2-devel@ml01.01.org>; Carsey, Jaben <jaben.carsey@intel.com>;
>>> Laszlo Ersek <lersek@redhat.com>
>>> Subject: Re: [edk2] [PATCH] GPT Shell Application/Library
>>>
>>> To the old question about license: I asked our people to check and was
>>> told that the license is compatible with our BSD and ok by Intel.
>>>
>>> To the technical content – I really like this idea of a shared library.
>>> That would be a great way to share code and not have as much duplicate.
>>>
>>> -Jaben
>>>
>>> From: Vladimir Olovyannikov
>>> [mailto:vladimir.olovyannikov@broadcom.com]
>>> Sent: Monday, October 17, 2016 10:52 AM
>>> To: Michael Zimmermann <sigmaepsilon92@gmail.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>; Carsey, Jaben
>>> <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
>>> edk2-devel@lists.01.org <edk2-devel@ml01.01.org>
>>> Subject: RE: [edk2] [PATCH] GPT Shell Application/Library
>>> Importance: High
>>>
>>> Hi Michael,
>>> I am absolutely agree with your proposal.
>>>
>>> In the gpt Shell library/application I had to “borrow” some stuff from
>>> PartitionDxe.c to not reinvent a  wheel.
>>> If the PartitionDxe maintainer agrees to have a separate library
>>> available for everybody, I would move all the GPT-related stuff from
>>> the GptWorker (and partially from the PartitionDxe itself) to that
>> independent library.
>>> This could be a longer-term task.
>>> Right now I just wanted to share the tool which could be useful for
>>> anybody who would wish to manage GPT partitions (and/or do a FAT32
>>> format of either a disk or a GPT partition) from within the Shell. What
>>> do
>> you think?
>>>
>>> Thank you,
>>> Vladimir
>>> From: Michael Zimmermann
>>> [mailto:sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>]
>>> Sent: October-17-16 12:25 AM
>>> To: Vladimir Olovyannikov
>>> Cc: Laszlo Ersek; Jaben Carsey; Ni, Ruiyu;
>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>> Subject: Re: [edk2] [PATCH] GPT Shell Application/Library
>>>
>>> Hi,
>>>
>>> wouldn't it be better to make a generic gpt parsing library which is
>>> independent of the shell so both the shell and PartitionDxe can use it?
>>> It may also be useful for other applications which need additional
>>> information like the gpt partition names.
>>>
>>> Thanks
>>> Michael
>>>
>>> On Mon, Oct 17, 2016 at 8:49 AM, Vladimir Olovyannikov
>>>
>> <vladimir.olovyannikov@broadcom.com<mailto:vladimir.olovyannikov@broa
>> d
>>> com.com>>
>>> wrote:
>>> Thank you Laszlo,
>>>
>>> Sorry, I missed the fields; it is my first contribution, I will add
>>> the required lines, review the source according to your comments and
>>> will resubmit the patch.
>>> So do you think the command should be _gpt instead of gpt? I was
>>> following TFTP and SF commands as a template.
>>>
>>> Thank you,
>>> Vladimir
>>>
>>> On Oct 16, 2016 1:05 PM, "Laszlo Ersek"
>>> <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
>>>>
>>>> On 10/16/16 07:23, Vladimir Olovyannikov wrote:
>>>>> This allows managing (create, delete, modify, fat format) of GPT
>>>>> partitions from within UEFI Shell.
>>>>> Syntax:
>>>>> gpt <command> [device_mapped_name] [parameters...] See usage
>>>>> examples in the .uni file
>>>>> ---
>>>>>  .../Library/UefiShellGptCommandLib/FatFormat.c     |  611 +++++++
>>>>>  .../Library/UefiShellGptCommandLib/FatFormat.h     |  111 ++
>>>>>  .../Library/UefiShellGptCommandLib/GptWorker.c     | 1902
>>> ++++++++++++++++++++
>>>>>  .../Library/UefiShellGptCommandLib/GptWorker.h     |  186 ++
>>>>>  .../UefiShellGptCommandLib.c                       | 1135
>>>>> ++++++++++++
>>>>>  .../UefiShellGptCommandLib.inf                     |   79 +
>>>>>  .../UefiShellGptCommandLib.uni                     |  117 ++
>>>>>  ShellPkg/ShellPkg.dec                              |    1 +
>>>>>  ShellPkg/ShellPkg.dsc                              |    4 +
>>>>>  9 files changed, 4146 insertions(+)  create mode 100644
>>>>> ShellPkg/Library/UefiShellGptCommandLib/FatFormat.c
>>>>>  create mode 100644
>>>>> ShellPkg/Library/UefiShellGptCommandLib/FatFormat.h
>>>>>  create mode 100644
>>>>> ShellPkg/Library/UefiShellGptCommandLib/GptWorker.c
>>>>>  create mode 100644
>>>>> ShellPkg/Library/UefiShellGptCommandLib/GptWorker.h
>>>>>  create mode 100644
>>> ShellPkg/Library/UefiShellGptCommandLib/UefiShellGptCommandLib.c
>>>>>  create mode 100644
>>> ShellPkg/Library/UefiShellGptCommandLib/UefiShellGptCommandLib.inf
>>>>>  create mode 100644
>>> ShellPkg/Library/UefiShellGptCommandLib/UefiShellGptCommandLib.uni
>>>>
>>>> This looks like a supremely welcome, long-awaited addition (latest
>>>> request:
>>>> <https://lists.01.org/pipermail/edk2-devel/2016-October/002667.html>)
>>>> , but it really needs your Signed-off-by, and the Contributed-under
>>>> line above it:
>>>>
>>>> ShellPkg/Contributions.txt
>>>>
>>>> I would also suggest (simply based on what I've seen elsewhere in
>>>> edk2) to keep the copyright notices tightly collected in the file
>>>> headings.
>>>>
>>>> Someone will have to go over all the licenses too -- does the
>>>> "Marvell BSD License Option" for example correspond to the 3-clause
>> BSDL?
>>>>
>>>> On the technical side, I believe that as long as a shell command (or
>>>> a command option) is not standardized (in the shell spec), it usually
>>>> starts with an underscore (_), so as to prevent future name collisions.
>>>> (I could be wrong about this -- I now recall the TFTP command, which
>>>> is also not in the 2.2 spec.)
>>>>
>>>> Just my two cents.
>>>>
>>>> Thanks
>>>> Laszlo
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>