From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9F34981DE5 for ; Fri, 28 Oct 2016 09:58:45 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP; 28 Oct 2016 09:58:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,410,1473145200"; d="scan'208";a="1077294563" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga002.fm.intel.com with ESMTP; 28 Oct 2016 09:58:46 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 28 Oct 2016 09:58:45 -0700 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.170]) by FMSMSX154.amr.corp.intel.com ([169.254.6.230]) with mapi id 14.03.0248.002; Fri, 28 Oct 2016 09:58:44 -0700 From: "Carsey, Jaben" To: Tim Lewis , "edk2-devel@lists.01.org" CC: "Carsey, Jaben" Thread-Topic: [shell] AliasLower never used in InternalSetAlias Thread-Index: AdIwlmPP8ooonpLXRf2RvwArhReYzQAAmqFgACVsmeAAAs2gQAAAmUHA Date: Fri, 28 Oct 2016 16:58:45 +0000 Message-ID: References: <7236196A5DF6C040855A6D96F556A53F3FD11D@msmail.insydesw.com.tw> <7236196A5DF6C040855A6D96F556A53F3FD173@msmail.insydesw.com.tw> <7236196A5DF6C040855A6D96F556A53F3FD93F@msmail.insydesw.com.tw> In-Reply-To: <7236196A5DF6C040855A6D96F556A53F3FD93F@msmail.insydesw.com.tw> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTQyYzM5MDAtMjJjNi00YzU2LThmMjktOTVmYTk2NDlmMzc1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IklmZzZnZFdTVUREUEYxV1FqUUt0bStQS1h3blpDSzZMbnpxaWN2Y1VzWXc9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Subject: Re: [shell] AliasLower never used in InternalSetAlias X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Oct 2016 16:58:45 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I agree with the goal of being more closely matched to the spec. I think that some things we need to adjust the spec, others we adjust the c= ode, and others may go beyond the spec. We have no prohibition that an imp= lementation cannot have additional features as long as they do not violate = the requirements of the spec. The unused code for #2 is a simple bug I think (as is any unused code I wou= ld argue). -Jaben > -----Original Message----- > From: Tim Lewis [mailto:tim.lewis@insyde.com] > Sent: Friday, October 28, 2016 9:49 AM > To: Carsey, Jaben ; edk2-devel@lists.01.org > Subject: RE: [shell] AliasLower never used in InternalSetAlias > Importance: High >=20 > Jaben -- >=20 > 1) I'm not talking about desired behavior, I'm talking about spec behavio= r. As > an external user of GetAlias() and SetAlias() (and a non-EDK2 shell > implementer), I have become aware of a number of places where the EDK2 > shell makes assumptions. The spec requirement is that commands are case- > insensitive, but does not place this requirement on Alias. The way in whi= ch > the command-line uses aliases is not relevant to how the API responds. Th= e > fact is: there might be entries for both Dir and dir, and the command-lin= e > usage in this case is non-deterministic as the specification sits now. Ob= viously > the spec needs an update to deal with what is currently a behavior which = can > lead to unexpected API failures. > 2) As I pointed out separately, the current code doesn't work as you (or = I) > describe. It does not use AliasLower (the lower case version of the input > string) for the variable name in SetAlias, but does use AliasLower in Get= Alias. > 3) The specification also states that aliases are identifiers, but the in= ternal > usage for "cd.." and "cd\" do not follow this rule. Neither the command o= r > the API enforce this rule. >=20 > BTW, another place I have found is in the mapping API, where the "cd" > command currently relies on the fact that the 2nd parameter can have a > mapping name in it (while leaving the first parameter as a NULL). This is > unexpected behavior on the API part. >=20 > Tim > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > Sent: Friday, October 28, 2016 8:20 AM > To: Tim Lewis ; edk2-devel@lists.01.org > Cc: Carsey, Jaben > Subject: RE: [shell] AliasLower never used in InternalSetAlias >=20 > Tim, >=20 > Given that all commands are case insensitive, I couldn't imagine why we > would want case-sensitive alias. >=20 > Do we really want "Dir" to fail, while "dir" works fine? Remember that t= he > real command is case insensitive "ls" in either case. >=20 > -Jaben >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Tim Lewis > > Sent: Thursday, October 27, 2016 2:29 PM > > To: Tim Lewis ; edk2-devel@lists.01.org > > Subject: Re: [edk2] [shell] AliasLower never used in InternalSetAlias > > Importance: High > > > > I would also note that GetAlias() has similar logic, but does, in fact > > use the AliasLower. As far as I can tell, the specification does not > > say anything about case-insensitive, so I believe this to be in error. > > > > Tim > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Tim Lewis > > Sent: Thursday, October 27, 2016 2:11 PM > > To: edk2-devel@lists.01.org > > Subject: [edk2] [shell] AliasLower never used in InternalSetAlias > > > > In the function InternalSetAlias, it appears that AliasLower is > > duplicated (fromAlias), converted to lower case and freed ,but never > > actually used. Am I missing something? > > > > // Convert to lowercase to make aliases case-insensitive > > if (Alias !=3D NULL) { > > AliasLower =3D AllocateCopyPool (StrSize (Alias), Alias); > > if (AliasLower =3D=3D NULL) { > > return EFI_OUT_OF_RESOURCES; > > } > > ToLower (AliasLower); > > } else { > > AliasLower =3D NULL; > > } > > > > // > > // We must be trying to remove one if Alias is NULL > > // > > if (Alias =3D=3D NULL) { > > // > > // remove an alias (but passed in COMMAND parameter) > > // > > Status =3D (gRT->SetVariable((CHAR16*)Command, &gShellAliasGuid, 0, > > 0, NULL)); > > } else { > > // > > // Add and replace are the same > > // > > > > // We dont check the error return on purpose since the variable > > may not exist. > > gRT->SetVariable((CHAR16*)Command, &gShellAliasGuid, 0, 0, NULL); > > > > Status =3D (gRT->SetVariable((CHAR16*)Alias, &gShellAliasGuid, > > > EFI_VARIABLE_BOOTSERVICE_ACCESS|(Volatile?0:EFI_VARIABLE_NON_VOL > > ATILE), StrSize(Command), (VOID*)Command)); > > } > > > > if (Alias !=3D NULL) { > > FreePool (AliasLower); > > } > > _______________________________________________ > > 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