grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] sparc64: fix OF path names for sun4v systems


From: Daniel Kiper
Subject: Re: [PATCH] sparc64: fix OF path names for sun4v systems
Date: Tue, 19 Dec 2017 21:56:53 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

On Mon, Dec 18, 2017 at 02:30:51PM -0700, Eric Snowberg wrote:
> > On Dec 18, 2017, at 8:22 AM, Daniel Kiper <address@hidden> 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 <address@hidden>
> >> ---
> >> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]