Re: [AD] load_datafile_object with properties

["Eric Botcazou" <ebotcazou@xxxxxxxxxx>]

> The load_datafile_object function doesn't read properties (it's not the
> most useful function, but I'd have some use for it now - if it would
> read properties) - and in my recent debugging adventures with datafiles
> I learned enough about the datafile format to make a patch. It got a bit
> longer than my expected 3 lines - and I wanted to avoid too much code
> duplication, therefore I also had to simplify the other datafile reading
> function a bit. Probably the reasons this wasn't fixed earlier :)

Looks good, except the following points:

+/* read_property:
+ *  Helper to read a property from a datafile, returns 0
+ *  on success.
+ */
+static int read_property(PACKFILE *f, DATAFILE_PROPERTY *prop)

The actual return values of the functions go against the header comment;
given Allegro's convention, the comment is right.

+   }
+
+   pack_fseek(f, size);
+   return 0;

   }
   else {
      *allegro_errno = ENOMEM;
      pack_fseek(f, size);
      return -1;
   }

Same thing for subsequent failures of malloc().

+/* add_property:
+ *  Helper to add a new property to a properties list. Returns a
+ *  pointer to the added property inside the list, or NULL on failure.
+ */
+static DATAFILE_PROPERTY *add_property(DATAFILE_PROPERTY **properties)

The name of the function and the header comment are misleading: the function
doesn't add a property to the list, it grows the property list (actually
rather a C++ vector-like container) and returns a pointer to the location of
the newly created empty member.

+   prop = realloc (*properties, sizeof(DATAFILE_PROPERTY)*(count+2));

Consider using _al_sane_realloc from src/libc.c

+/* read_single_object:
+ *  Reads a single object from a datafile.
+ */
+static void read_single_object(DATAFILE *dat, PACKFILE *f, int type)

Merge it with load_object().

+      if (type == DAT_PROPERTY)
+      {
+  if (read_property (f, &prop))
+  {
+     newprop = add_property (&properties);
+     if (newprop)
+        *newprop = prop;
   }

This chunk of code doesn't follow Allegro's coding conventions for {}. There
are some other places with this problem too.

+      /* both read_property and read_single_object use *allegro_errno
+         for error reporting */
       if (*allegro_errno) {
   unload_datafile(dat);
-
-  for (c=0; c<MAX_PROPERTIES; c++)
-     if (prop[c].dat)
-        free(prop[c].dat);
-
-  return NULL;
+  dat = NULL;
+  break;

You still have to free the property list here.

+   /* concatenate to filename#objectname, to avoid recursion later */
+   ustrzcpy(parent, sizeof(parent), filename);
+
+   if (ustrcmp(parent, uconvert_ascii("#", tmp)) != 0)
+      ustrzcat(parent, sizeof(parent), uconvert_ascii("#", tmp));
+
+   ustrzcat(parent, sizeof(parent), objectname);
+
+   /* separate into path and actual objectname (for nested files) */
+   prevptr = bufptr = parent;
+   while ((c = ugetx(&bufptr)) != 0) {
+      if ((c == '#') || (c == '/') || (c == OTHER_PATH_SEPARATOR)) {
+  separator = prevptr;
+      }
+      prevptr = bufptr;
+   }
+
+   ustrzcpy(child, sizeof(child), separator + uwidth (separator));

I don't understand: you concatenate then split at the concatenation point.
Aren't you guaranteed that child == objectname, even for nested datafiles,
given that objectname can't contain the # character ? Moreover, why to
compare against / and OTHER_PATH_SEPARATOR ?

+     else {
+        /* skip unwanted object */
+        size = pack_mgetl(f);
+        pack_fseek(f, size+4);
+        c++;
+     }

Add a comment for the 4.

> There also is no more MAX_PROPERTIES limit

Yes, but I'm not very fond of your half-cooked solution, i.e the
vector-like thingie. A plain linked-list would be more elegant, but I guess
we must be cautious here because the DATAFILE_PROPERTY structure appears to
be on the edge between the internal side and the public side: it is not
documented in the manual (and the type of DATAFILE.prop is documented as
void *), but it is declated in datafile.h (and the type of DATAFILE.prop is
DATAFILE_PROPERTY *).

[What do other developers think ? Can we add a 'next' member to the
DATAFILE_PROPERTY structure ?]

I think your patch is worth an entry in docs/src/api._tx documenting the
improvement for load_datafile_object().

--
Eric Botcazou