[Toybox] [TOY] lspci

[Sandeep Sharma]

On Tue, Jul 23, 2013 at 6:20 AM, Felix Janda <felix.janda at posteo.de> wrote:

>  Georgi Chorbadzhiyski wrote:
> > On 7/22/13 10:05 PM, Felix Janda wrote:
> > > Isaac wrote:
> > >>
> > >> I've written an lspci implementation.
> > >
> > > Cool.
> > >
> > >> Currently it supports -emkn; -e is an extension ("class" is a 24-bit
> number,
> > >> but lspci only shows 16 bits; one person on the Puppy forums mentioned
> > >> that they need those last 8 bits).
> > >> -n is a no-op for compatability with standard lspci.
> > >
> > > Are there any more options you have in mind that should be implemented?
> > > Just out of curiosity: Could you post a link to the forum post?
> > >
> > >> 75 lines total.
> > >
> > > Down to 70 after application of the below patch with small fixes.
> > >
> > > It felt more sensible to me to read 2 more bytes and ignore them than
> to
> > > seek for 2 bytes in a file. So preadat_name now has one argument less.
> > >
> > > Use calloc instead of malloc + memset. With this also the off-by-one
> > > error from (origninally) line 32 is fixed. It would be nice to use
> > > toybuf for this, wouldn't it? That would also make valgrind happier.
> > >
> > > Print "" instead of "." when .../driver is missing.
> > >
> > > The only difference between the usual output and the machine readable
> > > output is the format string. Use this to avoid some repetition.
> > >
> > > Felix
> > >
> > > --- a/lspci.c       2013-07-22 18:05:37.514838360 +0200
> > > +++ b/lspci.c       2013-07-22 19:12:03.000000000 +0200
> > > @@ -17,19 +17,14 @@
> > >  */
> > >  #define FOR_lspci
> > >  #include "toys.h"
> > > -char * preadat_name(int dirfd, char *fname, size_t nbyte, off_t
> offset)
> > > +char * preadat_name(int dirfd, char *fname, size_t nbyte)
> > >  {
> > >    int fd;
> > > -  char *buf = malloc(nbyte+1);
> > > -  memset(buf, 0, sizeof(buf));
> > > -  fd = openat(dirfd, fname, O_RDONLY);
> > > -  if (fd < 0) {
> > > -    return NULL;
> > > -  }
> > > -  lseek(fd, offset, SEEK_SET);
> > > +  char *buf = calloc(1, nbyte+1);
> > > +
> > > +  if (0 > (fd = openat(dirfd, fname, O_RDONLY))) return NULL;
> >
> > It would be better to move calloc after openat (which may fail).
>
> That's true. Actually I'd prefer having no dynamic allocation at all.
> Have something like
>
> struct {
>   char class[8], vendor[6], device[6], module[256];
> };
>
> at toybuf and use that as the buffer. Maybe also remove preadat
> all together and use a loop in do_lspci?
>
> Georgi Chorbadzhiyski wrote:
> > And another thing, IMHO
> >
> > ((fd = openat(dirfd, fname, O_RDONLY)) < 0) return NULL;
> >
> > is better than
> >
> > if (0 > (fd = openat(dirfd, fname, O_RDONLY))) return NULL;
>
> I agree.
>
> Felix
>  ______
>


  hi,
         I have some questions actually :-

 > > -  fd = openat(dirfd, fname, O_RDONLY);
> > -  if (fd < 0) {
> > -    return NULL;
> > -  }
> > -  lseek(fd, offset, SEEK_SET);
> > +  char *buf = calloc(1, nbyte+1);
> > +
            why calloc and why not __xzalloc__ ?  Are not we following
toybox coding style.

  And what if _calloc_ returns NULL. we will be  using a NULL buffer, it
may lead to a potentional crash.

--
Sandeep




>  _________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20130723/fa205273/attachment-0006.htm>