Hi Werner
> > there is also a problem in the plsetopts() code when calling device
> > options, that when you call this function twice it forgets about the
> > first call - and this may have to do what you are describing.
I think this is another symptom of the same problem.
I've had a closer look at the code. While drv_opt is a static global
variable used to store the first option, all other members of the linked
list anchored at drv_opt are dynamically allocated during option parsing by
opt_drvopt. A consequence of the lines
drv_opt.option = drv_opt.value = NULL;
drv_opt.next = NULL;
in c_plparseopts() is that any existing linked list anchored at drv_opt
is lost whenever plsetopt() is called.
I tested the theory with valgrind and it supports the assertion that there
is a memory leak if plsetopt() is called twice. If I deliberately call
plsetopt() twice with the same options a lost block is reported having been
created by the first call. No such error is reported by valgrind if
plsetopt() is only called once.
Another consequence of this is that I *think* any device options passed in
via the command line (if allowed) will be lost once the application calls
plsetopt().
As to how to solve this I can think of two options, neither of which is
entirely satisfactory. The first is to not zero out drv_opt in
c_plparseopts() and instead rely on this being done elsewhere (plinit()
perhaps). This would certainly mean that new arguments don't cause old ones
to be lost, but this is also a potential problem - sometimes a program will
want to *change* the value of options which might have been previously set
or even delete them altogether. Probably more importantly, it also means
that multiple calls to set device options via plsetopt() will cause the
device option list to grow without limit which is also undesireable.
Another alternative is to deallocate the device options list in
c_plparseopts(). This would give rise to the current behaviour except it
would prevent the memory leak. The loss of previously specified
device options may not be desireable in all situations either so this is
probably not a vialble long term solution.
Thinking about it some more, perhaps it is possible to treat device option
setting via plsetopt() as a merge operation rather than as an overwrite.
This will make opt_drvopt() more complex and slightly slower as it would
have to search the existing linear linked list to see if a parameter existed
before adding it to the list. However, it would make it possible to change
option values while preserving existing device options when adding new ones.
To this end I have attached an initial patch (against plplot 5.8.0) which
implements this last idea. Comments are welcome. In terms of deleting an
existing option, an option of
foobar
will retain the existing option and set the value to the default of 1, while
foobar=
should cause the existing option foobar to be deleted. If there is no
existing option foobar the delete operation is a no-op.
Regards
jonathan
--- src/plargs.c-orig 2007-11-19 06:33:59.000000000 +1030
+++ src/plargs.c 2008-03-03 13:16:22.000000000 +1030
@@ -624,7 +624,7 @@
} DrvOptCmd;
/* the variable where opt_drvopt() stores the driver specific command line
options */
-static DrvOptCmd drv_opt;
+static DrvOptCmd drv_opt = {NULL, NULL, NULL,};
static int tables = 1;
@@ -759,10 +759,6 @@
mode_nodash = mode & PL_PARSE_NODASH;
mode_skip = mode & PL_PARSE_SKIP;
- /* Initialize the driver specific option linked structure */
- drv_opt.option = drv_opt.value = NULL;
- drv_opt.next = NULL;
-
myargc = (*p_argc);
argend = argv + myargc;
@@ -1327,6 +1323,9 @@
free(drvpl);
} while(drvp != NULL);
+
+ drv_opt.option = NULL; drv_opt.value = NULL;
+ drv_opt.next = NULL;
}
@@ -1695,6 +1694,96 @@
}
/*--------------------------------------------------------------------------*\
+ * opt_drvopt_merge()
+ *
+ * Helper function for opt_drvopt(). Merge the given driver specific option
+ * into the driver option list. If value is NULL or zero length the
+ * corresponding option will be removed from the option list if present.
+ * This function is complicated somewhat through the use of a static root
+ * list node.
+ * Return value is 0 on success or 1 on error.
+\*--------------------------------------------------------------------------*/
+
+static int opt_drvopt_merge(const char *opt, const char *value)
+{
+ DrvOptCmd *drvp = &drv_opt, *prev = NULL;
+
+ while (drvp!=NULL && drvp->option!=NULL && strcmp(drvp->option, opt)) {
+ prev = drvp;
+ drvp = drvp->next;
+ }
+
+ /* If instruction is to delete an option but the option doesn't exist,
+ * just return.
+ */
+ if ((drvp==NULL || drvp->option==NULL) && (value==NULL || value[0]==0))
+ return 0;
+
+ if (drvp == NULL) {
+ /* New option, list has existing options. Put new option at end. */
+ char *c=NULL, *v=NULL;
+
+ drvp = (DrvOptCmd *) malloc(sizeof(DrvOptCmd)); /* don't release */
+ c = plstrdup(opt); /* it should not be release, because of familying */
+ v = plstrdup(value); /* don't release */
+ if (c==NULL || v==NULL || drvp==NULL) {
+ free(c);
+ free(v);
+ free(drvp);
+ return 1;
+ }
+ drvp->option = c;
+ drvp->value = v;
+ drvp->next = NULL;
+ prev->next = drvp;
+ } else {
+ if (drvp->option == NULL) {
+ /* New option, list is empty. Pop option into static list head. */
+ drvp->option = plstrdup(opt);
+ drvp->value = plstrdup(value);
+ drvp->next = NULL;
+ if (drvp->option==NULL || drvp->value==NULL) {
+ free(drvp->option);
+ free(drvp->value);
+ return 1;
+ }
+ } else
+ /* Option already exists */
+ if (value==NULL || value[0]==0) {
+ /* Delete option */
+ DrvOptCmd *next = drvp->next;
+ free(drvp->option);
+ free(drvp->value);
+ if (prev != NULL) {
+ /* Delete non-root node */
+ prev->next = next;
+ free(drvp);
+ } else {
+ /* Delete static root node contents */
+ if (next == NULL) {
+ drvp->option = NULL;
+ drvp->value = NULL;
+ } else {
+ drvp->option = next->option;
+ drvp->value = next->value;
+ free(drvp->next);
+ drvp->next = next->next;
+ }
+ }
+
+ } else {
+ /* Set new option value */
+ char *c = plstrdup(value);
+ if (c == NULL)
+ return 1;
+ free(drvp->value);
+ drvp->value = c;
+ }
+ }
+
+ return 0;
+}
+/*--------------------------------------------------------------------------*\
* opt_drvopt()
*
* Get driver specific options in the form <option[=value]>[,option[=value]]*
@@ -1706,7 +1795,7 @@
{
char t, *tt, *option, *value;
int fl = 0;
- DrvOptCmd *drvp;
+ DrvOptCmd *drvp = NULL;
option = (char *) malloc((size_t)(1+strlen(optarg))*sizeof(char));
if (option == NULL)
@@ -1716,7 +1805,6 @@
if (value == NULL)
plexit("opt_drvopt: Out of memory!?");
- drvp = &drv_opt;
*option = *value = '\0';
tt = option;
while((t = *optarg++)) {
@@ -1730,13 +1818,8 @@
}
*tt = '\0'; tt = option;
- drvp->option = plstrdup(option); /* it should not be release, because
of familying */
- drvp->value = plstrdup(value); /* don't release */
- drvp->next = (DrvOptCmd *) malloc(sizeof(DrvOptCmd)); /* don't release
*/
- if (drvp->next == NULL)
- plexit("opt_drvopt: Out of memory!?\n");
-
- drvp = drvp->next;
+ if (opt_drvopt_merge(option, value) != 0)
+ plexit("opt_drvopt: Out of memory!?\n");
break;
case '=':
@@ -1755,9 +1838,8 @@
value[1] = '\0';
}
- drvp->option = plstrdup(option); /* don't release */
- drvp->value = plstrdup(value); /* don't release */
- drvp->next = NULL;
+ if (opt_drvopt_merge(option, value) != 0)
+ plexit("opt_drvopt: Out of memory!?\n");
#ifdef DEBUG
fprintf(stderr, "\nopt_drvopt: -drvopt parsed options:\n");
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Plplot-devel mailing list
Plplot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/plplot-devel
