Re: [Patch 0/2] Add IPv6 support condition check for HTTP/ISCSI.

[Karunakar P <karunakarp@amiindia.co.in>]

Hi Jixian,

Thanks for your Detailed explanation.
Will review/Verify the changes and update you on Friday.

Thank You,
Karunakar
________________________________________
From: Wu, Jiaxin [jiaxin.wu@intel.com]
Sent: 18 October 2017 14:30
To: Karunakar P; edk2-devel@lists.01.org
Cc: Ye, Ting; Fu, Siyuan
Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for HTTP/ISCSI.

Hi Karunakar,

Actually, in my part, I didn't meet the ASSERT no matter the "Ipv6Available" returned from HttpBootCheckIpv6Support is true or false. According your ASSERT case that happened in Ip4DxeDriverBindingStart, which is caused by the FreePool of Private in Ip6DxeDriverBindingStart, so I guess the Ip6DxeDriverBindingStart may be involved ahead of Ip4DxeDriverBindingStart, then something wrong happened in Ip6DxeDriverBindingStart and goto the ON_ERROR (Private is freed here!). so, you can add the breakpoint within Ip6DxeDriverBindingStart/Ip4DxeDriverBindingStart to check it.

Per my analysis above, it does the issue that may trigger the potential ASSERT. So, I refined the patch as version2. The principle of patch v2 is that IPv6 and IPv4 should not affect each other even any failure happen, but the original code doesn't follow that:).


Thanks,
Jiaxin


> -----Original Message-----
> From: Karunakar P [mailto:karunakarp@amiindia.co.in]
> Sent: Wednesday, October 18, 2017 4:06 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for
> HTTP/ISCSI.
>
> Hi Jiaxin,
>
> 1. Cleaning the ConfigData when switching the mode will resolve the Issue A.
> 2. Will also verify the ASSERT issue and update you.
>
> Could you please help in clarifying the below items,
> 1. Could you please let us know why the ASSERT happens?
> 2. I've not faced any ASSERT with the changes attached in Bugzilla, Did you
> find any issues/drawbacks with that?
> -> In HttpBootCheckIpv6Support () definition Instead of getting Ipv6Support
> from Private->Nii->Ipv6Supported if we open the protocol there itself, Then
> there will not be issues in Destroying Children or FreePool(Private) right.
> Because we're going to check  HttpBootCheckIpv6Support() before opening
> any instances in HttpBootIp6DxeDriverBindingStart().
>
> Thanks for your great support.
>
> Thank You,
> Karunakar
> ________________________________________
> From: Wu, Jiaxin [jiaxin.wu@intel.com]
> Sent: 18 October 2017 12:35
> To: Karunakar P; edk2-devel@lists.01.org
> Cc: Ye, Ting; Fu, Siyuan
> Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for
> HTTP/ISCSI.
>
> Hi Karunakar,
>
> Thanks your verification. Base on your comments, I refined the series
> patches as below to fix the issues:
>
> [Patch v3 0/3] NetworkPkg/IScsiDxe: Display InitiatorInfo correctly.
>             NetworkPkg/IScsiDxe: Fix the incorrect/needless DHCP process.
>             NetworkPkg/IScsiDxe: Clean the previous ConfigData when switching
> the IP mode. /// This one is to fix the issue A.
>             NetworkPkg/IScsiDxe: Display InitiatorInfo in attempt page even DHCP
> enabled.
>
> [Patch v2 0/2] Add IPv6 support condition check.
>              NetworkPkg/HttpBootDxe: Add IPv6 support condition check. /// B
> && C have been fixed in version 2.
>              NetworkPkg/IScsiDxe: Add IPv6 support condition check.
>
> Please help to verify them.
>
> Best Regards,
> Jiaxin
>
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Karunakar P
> > Sent: Tuesday, October 17, 2017 6:13 PM
> > To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> > Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> > Subject: Re: [edk2] [Patch 0/2] Add IPv6 support condition check for
> > HTTP/ISCSI.
> >
> > Hi Jiaxin,
> >
> > I Reviewed the changes for 3 features/Bugs and verified the same, Please
> > find my below comments and issues faced
> >
> > A. Display InitiatorInfo in attempt page even DHCP enabled
> > -------------------------------------------------------------------------------------------
> ---
> > --------------------------------------------
> > 1. I applied IScsiConfigVfr.vfr  changes and as well IScsiMisc.c changes
> > 2. It displays initiator info properly when it's Enabled for DHCP
> > 3. But, I found some different behavior in below case
> >     a. Add  an Attempt (Attempt1  -> Initiator Info Enabled for DHCP)
> >     b. On reboot iSCSI  attempt was success and Initiator Details shown
> > properly ==> This is as expected
> >     c. Edit the same Attempt1 details to IP6 and save changes and reset
> >     d. Now Iscsi connection with IP6   ==> This is as Expected
> >     e. Now if we again Change the Attempt1 to IP4, It is Displaying Subnet
> > Mask ==> I guess we are not clearing It
> >
> >   I guess we need to do ZeroMem for initiator details before.
> >
> >
> > B. [Patch 2/2] NetworkPkg/IScsiDxe: Add IPv6 support condition check.
> > -------------------------------------------------------------------------------------------
> ---
> > -------------------------------------------
> > -> This changes looks similar whatever I attached in Bugzilla, and verified
> the
> > same with off board card witch doesn't support IP6
> > -> It works fine, I didn't find any issues on it.
> >
> >
> > C. [Patch 1/2] NetworkPkg/HttpBootDxe: Add IPv6 support condition check.
> > -------------------------------------------------------------------------------------------
> ---
> > ------------------------------------------------------------------------
> > I found some issues in this changes, please find my below comments
> > 1. HttpBootCheckIpv6Support() function definition and function call
> > parameter differs , To correct this I've done 1 insertion(+), 1 deletion(-) like
> > below
> > ...
> > +HttpBootCheckIpv6Support (
> > +  IN  EFI_HANDLE                   ControllerHandle,
> > +  IN  HTTP_BOOT_PRIVATE_DATA       *Private,
> > +  OUT BOOLEAN                      *Ipv6Support
> > +  )
> > ...
> > +  // Set IPv6 available flag.
> > +  //
> > +  Status = HttpBootCheckIpv6Support (ControllerHandle,
> > -This->DriverBindingHandle, &Ipv6Available);
> > +Private, &Ipv6Available);
> > ...
> >
> > 2. With the above changes I've verified with Off board card which doesn't
> > support IP6, But I'm facing below ASSERT
> > (324): CR has Bad Signature
> >
> > EFI_STATUS
> > EFIAPI
> > HttpBootIp4DxeDriverBindingStart (
> >   IN EFI_DRIVER_BINDING_PROTOCOL  *This,
> >   IN EFI_HANDLE                   ControllerHandle,
> >   IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath OPTIONAL
> >   )
> > {
> > ...
> >   if (!EFI_ERROR (Status)) {
> >     Private = HTTP_BOOT_PRIVATE_DATA_FROM_ID(Id);               // ASSERTs
> > here
> >   } else {
> > .....
> >
> > 3. I would like add some points and info about the this ASSERT, which I've
> > found
> > The ASSERT is happening because of FreePool (Private), mentioned exact
> > line no below
> >
> > EFI_STATUS
> > EFIAPI
> > HttpBootIp6DxeDriverBindingStart (
> >   IN EFI_DRIVER_BINDING_PROTOCOL  *This,
> >   IN EFI_HANDLE                   ControllerHandle,
> >   IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath OPTIONAL
> >   )
> > {
> > ...
> >   Status = gBS->InstallProtocolInterface (
> >                     &ControllerHandle,
> >                     &gEfiCallerIdGuid,
> >                     EFI_NATIVE_INTERFACE,
> >                     &Private->Id
> >                     );
> >     if (EFI_ERROR (Status)) {
> >       goto ON_ERROR;
> >     }
> >
> >   }
> >   +
> > +  //
> > +  // Set IPv6 available flag.
> > +  //
> > +  Status = HttpBootCheckIpv6Support (ControllerHandle,
> > -This->DriverBindingHandle, &Ipv6Available);
> > +Private, &Ipv6Available);
> > +  if (EFI_ERROR (Status)) {
> > +    //
> > +    // Fail to get the data whether UNDI supports IPv6.
> > +     // Set default value to TRUE.
> > +    //
> > +    Ipv6Available = TRUE;
> > +  }
> > +
> > +  if (!Ipv6Available) {
> > +    return EFI_UNSUPPORTED;
> > +  }
> >
> >    if (Private->Ip6Nic != NULL) {
> >      //
> > ...
> >
> > ON_ERROR:
> >
> >   HttpBootDestroyIp6Children(This, Private);
> >   HttpBootConfigFormUnload (Private);
> >   FreePool (Private);                                                                      // If I comment this
> > line ASSERT is not happening
> >
> >   return Status;
> > }
> >
> > 4. At your end could you please verify this IP6 Condition check for HTTP
> >
> > Please correct if anything is wrong, Thanks for your support
> >
> >
> > Thank You,
> > Karunakar
> >
> > -----Original Message-----
> > From: Wu, Jiaxin [mailto:jiaxin.wu@intel.com]
> > Sent: Tuesday, October 17, 2017 7:32 AM
> > To: Wu, Jiaxin; edk2-devel@lists.01.org
> > Cc: Karunakar P; Ye, Ting; Fu, Siyuan
> > Subject: RE: [edk2] [Patch 0/2] Add IPv6 support condition check for
> > HTTP/ISCSI.
> >
> > Hello Karunakar,
> >
> > Base on your original changes attached in Bugzilla 701
> > (https://bugzilla.tianocore.org/show_bug.cgi?id=710), I created the formal
> > series patches to support the IPv6 condition check for HTTP/ISCSI.
> >
> > Please help to review/verify it.
> >
> > BTW, To review the ISCSI part, please apply the "[Patch v2 0/2]
> > NetworkPkg/IScsiDxe: Display InitiatorInfo correctly" first to avoid any
> patch
> > conflict.
> >
> > Thanks,
> > Jiaxin
> >
> >
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > > Jiaxin Wu
> > > Sent: Tuesday, October 17, 2017 9:58 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Karunakar P <karunakarp@amiindia.co.in>; Ye, Ting
> > > <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
> > > <jiaxin.wu@intel.com>
> > > Subject: [edk2] [Patch 0/2] Add IPv6 support condition check for
> > HTTP/ISCSI.
> > >
> > > Base on the request of
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=710,
> > > we provide this patch to IPv6 condition check by leveraging AIP Protocol.
> > >
> > > Cc: Karunakar P <karunakarp@amiindia.co.in>
> > > Cc: Ye Ting <ting.ye@intel.com>
> > > Cc: Fu Siyuan <siyuan.fu@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Karunakar P <karunakarp@amiindia.co.in>
> > > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> > >
> > > Jiaxin Wu (2):
> > >   NetworkPkg/HttpBootDxe: Add IPv6 support condition check.
> > >   NetworkPkg/IScsiDxe: Add IPv6 support condition check.
> > >
> > >  NetworkPkg/HttpBootDxe/HttpBootDxe.c   | 115
> > > +++++++++++++++++++++++++++-
> > >  NetworkPkg/HttpBootDxe/HttpBootDxe.h   |   2 +
> > >  NetworkPkg/HttpBootDxe/HttpBootDxe.inf |   4 +-
> > >  NetworkPkg/IScsiDxe/IScsiConfig.c      |  18 +++++
> > >  NetworkPkg/IScsiDxe/IScsiDriver.c      |   2 +-
> > >  NetworkPkg/IScsiDxe/IScsiDriver.h      |   1 +
> > >  NetworkPkg/IScsiDxe/IScsiDxe.inf       |   2 +
> > >  NetworkPkg/IScsiDxe/IScsiImpl.h        |   1 +
> > >  NetworkPkg/IScsiDxe/IScsiMisc.c        | 135
> > > ++++++++++++++++++++++++++++++++-
> > >  NetworkPkg/IScsiDxe/IScsiMisc.h        |   4 +-
> > >  10 files changed, 278 insertions(+), 6 deletions(-)
> > >
> > > --
> > > 1.9.5.msysgit.1
> > >
> > > _______________________________________________
> > > 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