Re: [AD] grabber indexing patch

[ Thread Index | Date Index | More lists.liballeg.org/allegro-developers Archives ]


On Thu, 2002-09-19 at 14:23, Eric Botcazou wrote:
> > > Fine. Speaking of which, I would like to again modify the plugin
> > > interface (it was already for the 4.1.x series) in order to fix the
> > > problem Elias reported with errno being randomly set behind your back.
> >
> > Ok. I don't think I'll need to touch that code, but while I'm at it I
> > might as well have a look at that too. What did you have in mind?
> 
> Just changing the prototype for the save() method. But I'll do it myself.
> 

I assume, you want to change it so it has a return value indicating
failure?

Anyway, I just found some time to look at this, and considering the
severity this bug almost had for me (data loss), I propose the attached
patch. It mainly sets errno to 0 before calling the save() method, and
if the value changes, it sees that as failure indication.

I also added another errno = 0, which I think should be there. But of
course, if this is true, there are a lot of places, also in the main
code, where errno is treated wrong. Specifically, errno being set for
most libc functions doesn't indicate an error if the return value is 0.

For example, code like this is sometimes used:

int save (const char *filename)
{
  f = pack_fopen(filename, F_WRITE);
  if (!f)
    return *allegro_errno;  

  pack_iputc (0, f);

  some_other_function ();

  pack_iputc (0, f);

  pack_fclose(f);
  return *allegro_errno;
}

A way to fix it would be to find all occurences of errno in Allegro, and
change to this:

int save (const char *filename)
{
  f = pack_fopen(filename, F_WRITE);
  if (!f)
    return *allegro_errno;  

  *allegro_errno = 0; // in this example, already done by pack_fopen,
                      // but is not libc behaviour

  pack_iputc (0, f);

  save_errno = *allegro_errno;
  some_other_function (); // function which might change errno, but we
                          // don't care to what value
  *allegro_errno = save_errno;

  pack_iputc (0, f);

  pack_fclose(f);
  return *allegro_errno;
}

I'm not really happy with it though. Maybe it's better to completely
remove errno (except to check the type of error right after a libc
function call returns failure) - or at least, do this for Allegro 5?

I could try looking for errno in the sources and fix possible problems,
after I know which is the best solution  - but I don't have much time so
it could take a while :) Or maybe, Eric, this is what you are doing
already? :)
For now, I'm happy with my patch which seems to fix all known
errno-related problems.

--
Elias Pschernig
Index: datedit.c
===================================================================
RCS file: /cvsroot/alleg/allegro/tools/datedit.c,v
retrieving revision 1.21
diff -u -r1.21 datedit.c
--- datedit.c	1 Oct 2002 10:10:37 -0000	1.21
+++ datedit.c	2 Oct 2002 12:22:45 -0000
@@ -368,16 +368,18 @@
 
 
 /* saves an object */
-static void save_object(DATAFILE *dat, AL_CONST int *fixed_prop, int pack, int pack_kids, int strip, int sort, int verbose, PACKFILE *f)
+static int save_object(DATAFILE *dat, AL_CONST int *fixed_prop, int pack, int pack_kids, int strip, int sort, int verbose, PACKFILE *f)
 {
    int i;
    DATAFILE_PROPERTY *prop;
    void (*save)(DATAFILE *, AL_CONST int *, int, int, int, int, int, int, PACKFILE *);
+   int failure = 0;
 
    prop = dat->prop;
    datedit_sort_properties(prop);
 
    while ((prop) && (prop->type != DAT_END)) {
+      errno = 0;
       if (should_save_prop(prop->type, fixed_prop, strip)) {
 	 pack_mputl(DAT_PROPERTY, f);
 	 pack_mputl(prop->type, f);
@@ -388,7 +390,7 @@
 
       prop++;
       if (errno)
-	 return;
+	 return 1;
    }
 
    if (verbose)
@@ -414,13 +416,20 @@
       if (verbose)
 	 datedit_endmsg("");
 
+      errno = 0;
       save((DATAFILE *)dat->dat, fixed_prop, pack, pack_kids, strip, sort, verbose, FALSE, f);
+      if (errno)
+	 failure = 1;
 
       if (verbose)
 	 datedit_startmsg("End of %-21s", get_datafile_property(dat, DAT_NAME));
    }
-   else
+   else {
+      errno = 0;
       save(dat, fixed_prop, (pack || pack_kids), FALSE, strip, sort, verbose, FALSE, f);
+      if (errno)
+	 failure = 1;
+   }
 
    pack_fclose_chunk(f);
 
@@ -438,6 +447,8 @@
       file_datasize += 4;
    else
       file_datasize += _packfile_datasize;
+
+   return failure;
 }
 
 
@@ -457,9 +468,7 @@
    pack_mputl(extra ? size+1 : size, f);
 
    for (c=0; c<size; c++) {
-      save_object(dat+c, fixed_prop, pack, pack_kids, strip, sort, verbose, f);
-
-      if (errno)
+      if (save_object(dat+c, fixed_prop, pack, pack_kids, strip, sort, verbose, f))
 	 return;
    }
 }


Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/