silx-kit / silx

silx toolkit
http://www.silx.org/doc/silx/latest/
MIT License
128 stars 73 forks source link

[warning] silx/io/specfile/src/sfinit.c #2302

Open alexmarie78 opened 5 years ago

alexmarie78 commented 5 years ago

warning on compilation :

static void
sfAssignScanNumbers(SpecFile *sf) {

  int i;
  char *ptr;
  char buffer[50];
  char buffer2[50];

  register   ObjectList *object,
                        *object2;
  SpecScan              *scan,
                        *scan2;

  for ( object = (sf->list).first; object; object=object->next) {
        scan = (SpecScan *) object->contents;

        lseek(sf->fd,scan->offset,SEEK_SET);
        read(sf->fd,buffer,sizeof(buffer));
        buffer[49] = '\0';

        for ( ptr = buffer+3,i=0; *ptr != ' ';ptr++,i++) buffer2[i] = *ptr;

        buffer2[i] = '\0';

        scan->scan_no = atol(buffer2);
        scan->order   = 1;
        for ( object2 = (sf->list).first; object2 != object; object2=object2->next) {
            scan2 = (SpecScan *) object2->contents;
            if (scan2->scan_no == scan->scan_no) scan->order++;
        }
  }
}

The return of read() and lseek() are not checked. We want to fix this warning, shall we stop the for loop when the return value is -1 (failed) or shall we do something else. The return type of the C function is void. So maybe we need to setup an errno somewhere ?

t20100 commented 5 years ago

To me the clean way to improve it is that sfAssignScanNumbers returns a status code based on lseek/read return code and that SfOpen2 and SfUpdate which are calling it check the returned value and in case of error forward it through their int * error argument.

The conservative handling of lseek/read error event would be to set the returned status as error, skip the current scan but continue the loop.

vasole commented 5 years ago

@t20100

Perhaps it is not necessary to be conservative and one can stop the loop. I guess the returned values were never checked because they were not expected to fail given the fact the scan limits have been previously determined.

t20100 commented 5 years ago

Yes sounds reasonable. This case should only occur if the file is clipped in the meantime which should not occur under normal condition.