On Thu, Aug 05, 2021 at 06:22:24PM -0400, Stefan Berger wrote: > On 7/30/21 11:45 AM, Stefan Berger wrote: > > From: Stefan Berger <stef...@linux.ibm.com> > > > > Move some #defines from ieee1275.c into the common ieee1275.h > > header file. Adjust the case used in IHANDLE_INVALID to use > > proper ihandle_t. > > > > Signed-off-by: Stefan Berger <stef...@linux.ibm.com> > > --- > > grub-core/kern/ieee1275/ieee1275.c | 29 ++++++++++++----------------- > > include/grub/ieee1275/ieee1275.h | 3 +++ > > 2 files changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/grub-core/kern/ieee1275/ieee1275.c > > b/grub-core/kern/ieee1275/ieee1275.c > > index 86f81a3c4..8fe92274d 100644 > > --- a/grub-core/kern/ieee1275/ieee1275.c > > +++ b/grub-core/kern/ieee1275/ieee1275.c > > @@ -21,11 +21,6 @@ > > #include <grub/types.h> > > #include <grub/misc.h> > > -#define IEEE1275_PHANDLE_INVALID ((grub_ieee1275_cell_t) -1) > > -#define IEEE1275_IHANDLE_INVALID ((grub_ieee1275_cell_t) 0) > > -#define IEEE1275_CELL_INVALID ((grub_ieee1275_cell_t) -1) > > - > > - > > I think it's safer to keep these here because of how the sizes for the > grub_ieee1275_cell_t are defined: > > # grep -r grub_ieee1275_cell include/grub/ | grep typedef > > include/grub/powerpc/ieee1275/ieee1275.h:typedef grub_uint32_t > grub_ieee1275_cell_t; > include/grub/sparc64/ieee1275/ieee1275.h:typedef grub_uint64_t > grub_ieee1275_cell_t; > > # grep -r grub_ieee1275_phandle include/grub/ | grep typedef > include/grub/ieee1275/ieee1275.h:typedef grub_uint32_t > grub_ieee1275_phandle_t; > > # grep -r grub_ieee1275_ihandle include/grub/ | grep typedef > include/grub/ieee1275/ieee1275.h:typedef grub_uint32_t > grub_ieee1275_ihandle_t; > > # grep -r GRUB_IEEE1275_PH include/grub/| grep def > include/grub/ieee1275/ieee1275.h:#define GRUB_IEEE1275_PHANDLE_INVALID > ((grub_ieee1275_phandle_t) -1) > > Using the global GRUB_IEEE1275_PHANDLE_INVALID here would probably break > things on sparc64 if we now use 32bit '-1' rather than 64bit '-1'.
Well, it seems to me at least some code around phandle/ihandle in the grub-core/kern/ieee1275/ieee1275.c file is buggy. Please take a look, e.g., at grub_ieee1275_finddevice(). I think args.phandle should be defined as grub_ieee1275_phandle_t. Otherwise *phandlep = args.phandle assignment will work by chance only. So, I think your GRUB_IEEE1275_PHANDLE_INVALID change to grub_ieee1275_phandle_t is correct. However, IMO the code related to phandle/ihandle in the grub-core/kern/ieee1275/ieee1275.c file should be fixed first. May I ask you to do that? Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
