On Mon, Dec 18, 2017 at 02:30:51PM -0700, Eric Snowberg wrote: > > On Dec 18, 2017, at 8:22 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote: > > On Wed, Dec 06, 2017 at 03:53:30PM -0800, Eric Snowberg wrote: > >> Fix the Open Firmware (OF) path property for sun4v SPARC systems. > >> These platforms do not have a /sas/ within their path. Over time > >> different OF addressing schemes have been supported. There > >> is no generic addressing scheme that works across every HBA. > >> > >> Signed-off-by: Eric Snowberg <eric.snowb...@oracle.com> > >> --- > >> grub-core/osdep/linux/ofpath.c | 147 > >> ++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 144 insertions(+), 3 deletions(-) > >> > >> diff --git a/grub-core/osdep/linux/ofpath.c > >> b/grub-core/osdep/linux/ofpath.c > >> index 3a8bc95a9..525a42ae6 100644 > >> --- a/grub-core/osdep/linux/ofpath.c > >> +++ b/grub-core/osdep/linux/ofpath.c > >> @@ -38,6 +38,44 @@ > >> #include <errno.h> > >> #include <ctype.h> > >> > >> +#ifdef __sparc__ > > > > This is not good. It will not work if you cross compile. > > What error do you see on a cross compile? I see __sparc__ used throughout > the code.
It seems to me that Vladimir (CC-ed) complained about that somewhere. However, it looks that this check is OK. So, if Vladimir is OK with that we can leave it as is. [...] > >> +static void > >> +check_hba_identifiers (const char *sysfs_path, int *vendor, int > >> *device_id) > >> +{ > >> + char *ed = strstr (sysfs_path, "host"); > >> + size_t path_size; > >> + char *p = NULL, *path = NULL; > > > > I think that you do not need to initialize *p here. > > I’ll remove the initialization. Thanks! > >> + char buf[8]; > >> + int fd; > >> + > >> + if (!ed) > >> + return; > >> + > >> + p = xstrdup (sysfs_path); > > > > Why do you need to duplicate sysfs_path? > > I do not think it is needed. Just p = sysfs_path? > > This is duplicated since the result of p is modified ... > > > > >> + ed = strstr (p, "host"); > >> + > >> + if (!ed) > >> + goto out; > >> + > >> + *ed = '\0’; > > right here. If I didn’t duplicate the string, it would corrupt > the sysfs_path. Also, sysfs_path is defined as const char *. Ahhh... Right I have missed that. Sorry. > >> + > >> + path_size = (strlen (p) + sizeof ("vendor")); > >> + path = xmalloc (path_size); > >> + > >> + if (!path) > >> + goto out; > >> + > >> + snprintf (path, path_size, "%svendor", p); > >> + fd = open (path, O_RDONLY); > >> + > >> + if (fd < 0) > >> + goto out; > >> + > >> + memset (buf, 0, sizeof (buf)); > >> + > >> + if (read (fd, buf, sizeof (buf) - 1) < 0) > >> + goto out; > >> + > >> + close (fd); > >> + sscanf (buf, "%x", vendor); > > > > Please add empty line here. > > > >> + snprintf (path, path_size, "%sdevice", p); > > > > I have a feeling that p should be changed somehow here > > or to be precise a bit earlier... Should not it? > > It is being changed above? *ed is a pointer within it. Yes, but only once. Later string pointed by p is not changed. The same value is used for "%svendor" and "%sdevice". Is it correct? Am I missing something? [...] > > In general I am not happy with sscanf() usage as a string to number > > converter. Especially without any error checking. However, as I can > > see, it is common here. So, I will accept fixed patch with sscanf() > > eventually but I would be happy if you replace it everywhere with > > something more robust in separate patch. > > I could look at doing that in the future. I always try to follow the > coding style within the file. Great! Thanks! Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel