[resubmit] memory leak, mi-cmd-var

[resubmit] memory leak, mi-cmd-var

[msnyder]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

After feedback from Nick Roberts -- it doesn't seem as if there is
any reason for the strdup.

I left the test for NULL in place, because I can't convince myself
that it is un-necessary.  Therefore the conservative approach is to
leave it in.  I've moved it to earlier so that if it's goint to error,
we won't do unnecessary work.

Version 1.1 of the file has this NULL check in it, right after the
xstrdup.  I would presume that Andrew Cagney would know that xstrdup
won't return if it fails, so I can't explain the test away by saying
he was just checking for xstrdup failure.


[-- Attachment #2: formspec2.txt --]
[-- Type: text/plain, Size: 1642 bytes --]

2007-06-28  Michael Snyder  <msnyder@access-company.com>

	* mi/mi-cmd-var.c (mi_cmd_var_set_format): Eliminate unnecessary
	xstrdup.

Index: mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.33
diff -p -r1.33 mi-cmd-var.c
*** mi/mi-cmd-var.c	13 Jun 2007 19:08:47 -0000	1.33
--- mi/mi-cmd-var.c	29 Jun 2007 04:27:28 -0000
*************** mi_cmd_var_set_format (char *command, ch
*** 203,218 ****
    if (argc != 2)
      error (_("mi_cmd_var_set_format: Usage: NAME FORMAT."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
  
    if (var == NULL)
      error (_("mi_cmd_var_set_format: Variable object not found"));
  
!   formspec = xstrdup (argv[1]);
!   if (formspec == NULL)
!     error (_("mi_cmd_var_set_format: Must specify the format as: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\""));
! 
    len = strlen (formspec);
  
    if (strncmp (formspec, "natural", len) == 0)
--- 203,218 ----
    if (argc != 2)
      error (_("mi_cmd_var_set_format: Usage: NAME FORMAT."));
  
+   if (argv[1] == NULL)
+     error (_("mi_cmd_var_set_format: Must specify the format as: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\""));
+ 
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
  
    if (var == NULL)
      error (_("mi_cmd_var_set_format: Variable object not found"));
  
!   formspec = argv[1];
    len = strlen (formspec);
  
    if (strncmp (formspec, "natural", len) == 0)

Re: [resubmit] memory leak, mi-cmd-var

[Nick Roberts]

 > I left the test for NULL in place, because I can't convince myself
 > that it is un-necessary.

mi_cmd_var_set_format (char *command, char **argv, int argc) is called like
main (char **argv, int argc).  So if argc=2, which it must be at this point,
then argv[1] can't be NULL can it?  If it is, there must be something seriously
wrong with the arguments of its caller, mi_cmd_execute (elements of struct
mi_parse *parse), and other MI commands will surely fail too.

 >                            Therefore the conservative approach is to
 > leave it in.  I've moved it to earlier so that if it's goint to error,
 > we won't do unnecessary work.

The conservative approach would be to wait until after the release, or at
least the branch/branchpoint.

In Emacs we went through a similar process before the release.  Dan Nicolaescu
reviewed the error list and submitted errors that he thought might cause
problems.  Members of the mailing list would comment as to whether they thought
a change was need or not.  Patches were only submitted once it was agreed that
there was a potential problem.  At least that's my recollection - I didn't get
involved.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [resubmit] memory leak, mi-cmd-var

[msnyder]

> The conservative approach would be to wait until after the release, or at
> least the branch/branchpoint.

That's perfectly reasonable -- I certainly don't object to holding these
patches for after the branchpoint, and I won't check any more in as
"obvious".

Just let's not have them fall thru a crack...   ;-)

Michael