Re: Functionality issues in UDF support

[Paulo Alcantara <pcacjr@zytor.com>]

On September 15, 2017 4:10:10 AM GMT-03:00, "Ni, Ruiyu" <ruiyu.ni@intel.com> wrote:
>Paulo,
>Can current code support multiple volumes in CDROM?

No. It can handle only a single UDF logical volume. I think I never found an UDF disk or image with more than a LVD (Logical Volume Descriptior). Although I think I also added some comment in the code that it currently handles only one logical volume per disk (no access to the code right now, sorry). Other than that, there should be also a DEBUG() or perhaps an ASSERT() in case more than one is found.

For supporting multiple logical volumes, we would have to follow your suggestion and change Partition driver to install a child handle for each LVD found in an UDF fs.

Thanks!
Paulo

>
>Thanks/Ray
>
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Friday, September 15, 2017 2:26 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Zeng,
>Star
>> <star.zeng@intel.com>
>> Subject: RE: Functionality issues in UDF support
>> 
>> 
>> 
>> Ray,
>> 
>> On September 15, 2017 3:02:11 AM GMT-03:00, "Ni, Ruiyu"
>> <ruiyu.ni@intel.com> wrote:
>> >Paulo,
>> >When will you change the implementation to this way?
>> 
>> Unfortunately I can't promise you when I'd be doing this because, you
>know,
>> I can only work on this in my free time (very short, ATM). Perhaps
>this
>> weekend - but real life also happens and I may get preempted by
>something
>> else.
>> 
>> Paulo
>> 
>> >
>> >
>> >Thanks/Ray
>> >
>> >> -----Original Message-----
>> >> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> >> Sent: Friday, September 15, 2017 1:38 PM
>> >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek
><lersek@redhat.com>
>> >> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>; Zeng,
>> >Star
>> >> <star.zeng@intel.com>
>> >> Subject: RE: Functionality issues in UDF support
>> >>
>> >>
>> >>
>> >> Hi,
>> >>
>> >> On September 15, 2017 2:08:28 AM GMT-03:00, "Ni, Ruiyu"
>> >> <ruiyu.ni@intel.com> wrote:
>> >> >Hi Paulo,
>> >> >The responsibility of PartitionDxe driver is to partition the
>> >physical
>> >> >BLK (the entire CDROM) into logical BLKs (one BLK for each
>volume).
>> >> >It doesn't make sense to create a child BLK handle which still
>> >covers
>> >> >the entire CDROM.
>> >> >So as I suggested in below, PartitionInstallUdfChildHandles()
>should
>> >> >contain a for-loop to iterate all the volumes in the CDROM, and
>> >create
>> >> >child BLK handle for each volumes, but skipping Eltorito volume.
>> >>
>> >> Ah, makes sense to me now. Thanks for clarifying it. So I agree
>with
>> >you.
>> >>
>> >> In whatever you guys decide to do with it, count on me to give
>some
>> >help.
>> >>
>> >> Thanks!
>> >> Paulo
>> >>
>> >> >
>> >> >Thanks/Ray
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> >> >> Sent: Friday, September 15, 2017 12:58 PM
>> >> >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek
>> ><lersek@redhat.com>
>> >> >> Cc: edk2-devel@lists.01.org; Wu, Hao A <hao.a.wu@intel.com>;
>Zeng,
>> >> >Star
>> >> >> <star.zeng@intel.com>
>> >> >> Subject: Re: Functionality issues in UDF support
>> >> >>
>> >> >>
>> >> >>
>> >> >> Hi Ray,
>> >> >>
>> >> >> On September 15, 2017 12:33:04 AM GMT-03:00, "Ni, Ruiyu"
>> >> >> <ruiyu.ni@intel.com> wrote:
>> >> >> >Paulo,
>> >> >> >Before raising my questions, I'd like to confirm that for a
>> >single
>> >> >> >CD/DVD in UDF format, there might be multiple volumes.
>> >> >> >One of the volume could be Eltorito type.
>> >> >> >If my understanding is correct, please continue reading below.
>> >> >>
>> >> >> Right.
>> >> >>
>> >> >> >
>> >> >> >We found below mapping table using "map -r" shell command in a
>> >> >platform
>> >> >> >with only PartitionDxe change and without UdfDxe driver.
>> >> >> >It's a bug that <BLK6> and <BLK7/FS2> are created. Actually
>they
>> >are
>> >> >> >identical to <BLK3> and <BLK4/FS1>.
>> >> >> >
>> >> >> >--- Mapping table---
>> >> >> >     BLK2: Alias(s):
>> >> >> >          PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)
>> >> >> >
>> >> >> >     BLK3: Alias(s):
>> >> >> >
>> >PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/CDROM(0x0)
>> >> >> >      FS1: Alias(s):CD1a65535a1:;BLK4:
>> >> >> >
>> >PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/CDROM(0x1)
>> >> >> >
>> >> >> >     BLK5: Alias(s):
>> >> >>
>> >>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/VenMedia(C5BD4D42-
>> >> >> 1A76-4996-8956-73CDA326CD0A)
>> >> >> >     BLK6: Alias(s):
>> >> >>
>> >>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/VenMedia(C5BD4D42-
>> >> >> 1A76-4996-8956-73CDA326CD0A)/CDROM(0x0)
>> >> >> >      FS2: Alias(s):CD1a65535ab:;BLK7:
>> >> >>
>> >>PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/VenMedia(C5BD4D42-
>> >> >> 1A76-
>> >> >> >4996-8956-73CDA326CD0A)/CDROM(0x1)
>> >> >> >--- End of mapping table ---
>> >> >> >
>> >> >> >After investigation, I found the UDF logic in Partition driver
>> >> >doesn't
>> >> >> >truly skip the Eltorito volume.
>> >> >> >The code flow is like below:
>> >> >> >
>> >> >> >  1.  <BLK2> is created by ScsiDiskDxe driver.
>> >> >> >  2.  By passing <BLK2> to PartitionDxe Start()
>> >> >> >*   <BLK3> and <BLK4/FS1> are created by PartitionDxe driver,
>by
>> >> >> >PartitionInstallElToritoChildHandles().
>> >> >> >*   <BLK5> is created by PartitionDxe driver, by
>> >> >> >PartitionInstallUdfChildHandles().
>> >> >> >  3.  By passing <BLK5> to PartitionDxe Start()
>> >> >> >*   <BLK6> and <BLK7/FS2> are created by PartitionDxe driver,
>by
>> >> >> >PartitionInstallElToritoChildHandles().
>> >> >> >
>> >> >> >I think step 2.a is not correct if my understanding to UDF is
>> >> >correct.
>> >> >> >The PartitionInstallUdfChildHandles() should iterate all
>volumes
>> >in
>> >> >the
>> >> >> >media and creates the child BLK handle for each volume, but
>> >skipping
>> >> >> >Eltorito volume.
>> >> >> >
>> >> >> >Instead, the current implementation just creates one child BLK
>> >> >handle
>> >> >> >for the entire media.
>> >> >> >To avoid reclusively creating  child BLK handle, the
>> >> >> >PartitionInstallUdfChildHandles() contains a logic to do
>nothing
>> >> >when
>> >> >> >the handle is created by ParititonDxe driver. The logic is not
>> >> >needed
>> >> >> >when the implementation follows my suggestion above.
>> >> >> >Due to this, step 3.a creates the additional but
>shouldn't-exist
>> >BLK
>> >> >> >handles <BLK6> and <BLK7/FS2>.
>> >> >> >
>> >> >> >UdfDxe driver is supposed to Start() on each volume and
>produce
>> >> >> >SimpleFileSystem protocol.
>> >> >>
>> >> >> It seems that PartitionInstallUdfChildHandles() indeed skips
>the
>> >> >ElTorito
>> >> >> partitions otherwise you'd see the ".../CDROM(0x1)/VenMedia()"
>and
>> >> >> ".../CDROM(0x0)/VenMedia()" device paths *also* in the mapping
>> >> >output.
>> >> >>
>> >> >> If I understand correctly, the problem is that when we create a
>> >child
>> >> >handle
>> >> >> for an UDF volume, Partition driver will execute again, parse
>the
>> >> >newly
>> >> >> created UDF handle, find again an ElTorito partition and then
>> >install
>> >> >a new
>> >> >> child handle (VenMedia()/CDROM())
>> >> >>
>> >> >> So, the logic of skipping of ElTorito partitions in
>> >> >> PartitionInstallUdfChildHandles() is not enough. Unfortunately
>we
>> >> >can't
>> >> >> handle UDF bridge disks (ElTorito + UDF) entirely in
>> >> >> PartitionInstallUdfChildHandles() -- we should probably also
>skip
>> >UDF
>> >> >device
>> >> >> paths in PartitionInstallElToritoChildHandles().
>> >> >>
>> >> >> Does it make sense to you, Ray? Thanks for raising this up.
>> >> >>
>> >> >> Paulo
>> >> >>
>> >> >> >
>> >> >> >Do you agree with my above suggestions?
>> >> >> >
>> >> >> >Laszlo,
>> >> >> >I understood your needs of this UDF support. But as you can
>see
>> >> >there
>> >> >> >are many build failures and even functionality issues due to
>this
>> >> >> >support.
>> >> >> >I am not sure how the other open source project handles such
>> >cases.
>> >> >> >But I am thinking maybe we could move the whole UDF support to
>> >> >> >edk2-staging firstly and move it back after all the issues are
>> >> >> >resolved.
>> >> >> >What's your suggestion?
>> >> >> >
>> >> >> >Thanks/Ray
>> >> >>
>> >> >> --
>> >> >> Sent from my Android device with K-9 Mail. Please excuse my
>> >brevity.
>> >>
>> >> --
>> >> Sent from my Android device with K-9 Mail. Please excuse my
>brevity.
>> 
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.