Hi Jonathan,
thanks again for your comments and for the patches. As usual I already
worked on a fix and in fact made nearly the identical changes to the
code as you suggested (commited to svn just now). About the merging
the options - this we should discuss here on the list. I conclude now
everything and ask the list for comments:
Before the commit:
- every time plsetopt was called the saved (command line options
and options set with plsetopt) *driver* options were forgotten
(leading to a memory leak) even if you didn't set driver options but
other options like "geometry"
After the commit:
- every time driver option are set (at command line or with
plsetopt()) driver options set before are "forgotten" (without memory
leak). If other options (e.g. plsetopt("geometry","800x600")) are set
which are not driver specific the driver options are still "remembered".
The only problem now is, that you can't delete already set drv_opt
without setting new ones - but some drivers don't take any specific
driver options. Jonathan, your merge code contradicts the driver
options policy, since as far as I understand it, the following code >>
plsetopt( "drvopt", "text" ); << would cause the option text to be
deleted if it was already set. But such code should actually set the
driver option text to the value 1. Also, I'm not sure if merging is
needed (it was also my first thought in the beginning), since the
options for the different drivers are not identical anyway, so for a
new driver you need to clean the options table anyway.
So, I'm not sure if it makes sense to merge options. Still now it's
not possible to clean the drv_opt table which I think is necessary.
Anyway, before I proceed here and look for a solution, I would like to
ask the list what the wanted behavior should be?
Regards,
Werner
On 03.03.2008, at 03:52, Jonathan Woithe wrote:
> 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");
--
Dr. Werner Smekal
Institut fuer Allgemeine Physik
Technische Universitaet Wien
Wiedner Hauptstr 8-10
A-1040 Wien
Austria
email: [EMAIL PROTECTED]
web: http://www.iap.tuwien.ac.at/~smekal
phone: +43-(0)1-58801-13463 (office), +43-(0)1-58801-13469 (laboratory)
fax: +43-(0)1-58801-13499
-------------------------------------------------------------------------
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
