[2/3] Factor out the code to do the datadir-relocation for gdbinit

Message ID 20190820221745.147370-3-cbiesinger@google.com
State Superseded
Headers show
Series
  • Load gdbinit files from a directory
Related show

Commit Message

Hannes Domani via Gdb-patches Aug. 20, 2019, 10:17 p.m.
gdb/ChangeLog:

2019-08-20  Christian Biesinger  <cbiesinger@google.com>

	* main.c (relocate_gdbinit_path_maybe_in_datadir): New function.
	(get_init_files): Update.
---
 gdb/main.c | 68 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 31 deletions(-)

-- 
2.23.0.rc1.153.gdeed80330f-goog

Comments

Sergio Durigan Junior Aug. 21, 2019, 5:19 p.m. | #1
On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> gdb/ChangeLog:

>

> 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

>

> 	* main.c (relocate_gdbinit_path_maybe_in_datadir): New function.

> 	(get_init_files): Update.


I'm afraid you'll need a descriptive commit message :-).

> ---

>  gdb/main.c | 68 +++++++++++++++++++++++++++++-------------------------

>  1 file changed, 37 insertions(+), 31 deletions(-)

>

> diff --git a/gdb/main.c b/gdb/main.c

> index b9e12589ab..a1d1904c9b 100644

> --- a/gdb/main.c

> +++ b/gdb/main.c

> @@ -191,6 +191,41 @@ relocate_gdb_directory (const char *initial, int flag)

>    return dir;

>  }

>  

> +static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)


You should break the line after 'std::string':

  static std::string
  relocate_gdbinit_path_maybe_in_datadir (std::string file)

> +{

> +  int datadir_len = strlen (GDB_DATADIR);


size_t.

Also, you could declare a return variable here and just fill it
inside each 'if', instead of returning early (and then having to return
an empty string at the end (but that's a matter of style, I know).

> +

> +  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory

> +     has been provided, search for SYSTEM_GDBINIT there.  */

> +  if (gdb_datadir_provided

> +      && datadir_len < file.length ()

> +      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0

> +      && IS_DIR_SEPARATOR (file[datadir_len]))

> +    {

> +      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR

> +	 to gdb_datadir.  */

> +

> +      size_t start = datadir_len;

> +      for (; IS_DIR_SEPARATOR (file[start]); ++start)

> +	continue;


Same comment here: this loop seems strange (starting from 'start').

> +      return std::string (gdb_datadir) + SLASH_STRING +

> +	file.substr(start);

> +    }

> +  else

> +    {

> +      char *relocated = relocate_path (gdb_program_name,

> +				       file.c_str(),

> +				       SYSTEM_GDBINIT_RELOCATABLE);

> +      if (relocated != nullptr)

> +	{

> +	  std::string retval(relocated);


Space between variable name and open parenthesis.

> +	  xfree (relocated);

> +	  return retval;

> +	}

> +    }

> +    return "";

> +}

> +

>  /* Compute the locations of init files that GDB should source and

>     return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If

>     there is no system gdbinit (resp. home gdbinit and local gdbinit)

> @@ -212,37 +247,8 @@ get_init_files (std::string *system_gdbinit,

>  

>        if (SYSTEM_GDBINIT[0])

>  	{

> -	  int datadir_len = strlen (GDB_DATADIR);

> -	  int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);

> -	  std::string relocated_sysgdbinit;

> -

> -	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory

> -	     has been provided, search for SYSTEM_GDBINIT there.  */

> -	  if (gdb_datadir_provided

> -	      && datadir_len < sys_gdbinit_len

> -	      && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0

> -	      && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))

> -	    {

> -	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR

> -		 to gdb_datadir.  */

> -

> -	      size_t start = datadir_len;

> -	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)

> -		continue;

> -	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +

> -		&SYSTEM_GDBINIT[start];

> -	    }

> -	  else

> -	    {

> -	      char *relocated = relocate_path (gdb_program_name,

> -					       SYSTEM_GDBINIT,

> -					       SYSTEM_GDBINIT_RELOCATABLE);

> -	      if (relocated != nullptr)

> -	        {

> -		  relocated_sysgdbinit = relocated;

> -		  xfree (relocated);

> -		}

> -	    }

> +	  std::string relocated_sysgdbinit =

> +	    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);

>  	  if (!relocated_sysgdbinit.empty () &&

>  	      stat (relocated_sysgdbinit.c_str (), &s) == 0)

>  	    sysgdbinit = relocated_sysgdbinit;

> -- 

> 2.23.0.rc1.153.gdeed80330f-goog


Otherwise, LGTM.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Hannes Domani via Gdb-patches Aug. 21, 2019, 5:43 p.m. | #2
On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
>

> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

>

> > gdb/ChangeLog:

> >

> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

> >

> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.

> >       (get_init_files): Update.

>

> I'm afraid you'll need a descriptive commit message :-).


Changed to:

2019-08-20  Christian Biesinger  <cbiesinger@google.com>

        * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
        out of get_init_files.
        (get_init_files): Update.

(I guess the title of the commit message is not enough?)

> > ---

> >  gdb/main.c | 68 +++++++++++++++++++++++++++++-------------------------

> >  1 file changed, 37 insertions(+), 31 deletions(-)

> >

> > diff --git a/gdb/main.c b/gdb/main.c

> > index b9e12589ab..a1d1904c9b 100644

> > --- a/gdb/main.c

> > +++ b/gdb/main.c

> > @@ -191,6 +191,41 @@ relocate_gdb_directory (const char *initial, int flag)

> >    return dir;

> >  }

> >

> > +static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)

>

> You should break the line after 'std::string':

>

>   static std::string

>   relocate_gdbinit_path_maybe_in_datadir (std::string file)


Thanks, done and also changed std::string to const std::string&.

> > +{

> > +  int datadir_len = strlen (GDB_DATADIR);

>

> size_t.

>

> Also, you could declare a return variable here and just fill it

> inside each 'if', instead of returning early (and then having to return

> an empty string at the end (but that's a matter of style, I know).


OK, if you prefer, sure. Done.

> > +

> > +  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory

> > +     has been provided, search for SYSTEM_GDBINIT there.  */

> > +  if (gdb_datadir_provided

> > +      && datadir_len < file.length ()

> > +      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0

> > +      && IS_DIR_SEPARATOR (file[datadir_len]))

> > +    {

> > +      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR

> > +      to gdb_datadir.  */

> > +

> > +      size_t start = datadir_len;

> > +      for (; IS_DIR_SEPARATOR (file[start]); ++start)

> > +     continue;

>

> Same comment here: this loop seems strange (starting from 'start').


See my response in the other thread.

> > +      return std::string (gdb_datadir) + SLASH_STRING +

> > +     file.substr(start);

> > +    }

> > +  else

> > +    {

> > +      char *relocated = relocate_path (gdb_program_name,

> > +                                    file.c_str(),

> > +                                    SYSTEM_GDBINIT_RELOCATABLE);

> > +      if (relocated != nullptr)

> > +     {

> > +       std::string retval(relocated);

>

> Space between variable name and open parenthesis.


Thanks! (Though irrelevant with the other change now)

> > +       xfree (relocated);

> > +       return retval;

> > +     }

> > +    }

> > +    return "";

> > +}

> > +

> >  /* Compute the locations of init files that GDB should source and

> >     return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If

> >     there is no system gdbinit (resp. home gdbinit and local gdbinit)

> > @@ -212,37 +247,8 @@ get_init_files (std::string *system_gdbinit,

> >

> >        if (SYSTEM_GDBINIT[0])

> >       {

> > -       int datadir_len = strlen (GDB_DATADIR);

> > -       int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);

> > -       std::string relocated_sysgdbinit;

> > -

> > -       /* If SYSTEM_GDBINIT lives in data-directory, and data-directory

> > -          has been provided, search for SYSTEM_GDBINIT there.  */

> > -       if (gdb_datadir_provided

> > -           && datadir_len < sys_gdbinit_len

> > -           && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0

> > -           && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))

> > -         {

> > -           /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR

> > -              to gdb_datadir.  */

> > -

> > -           size_t start = datadir_len;

> > -           for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)

> > -             continue;

> > -           relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +

> > -             &SYSTEM_GDBINIT[start];

> > -         }

> > -       else

> > -         {

> > -           char *relocated = relocate_path (gdb_program_name,

> > -                                            SYSTEM_GDBINIT,

> > -                                            SYSTEM_GDBINIT_RELOCATABLE);

> > -           if (relocated != nullptr)

> > -             {

> > -               relocated_sysgdbinit = relocated;

> > -               xfree (relocated);

> > -             }

> > -         }

> > +       std::string relocated_sysgdbinit =

> > +         relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);

> >         if (!relocated_sysgdbinit.empty () &&

> >             stat (relocated_sysgdbinit.c_str (), &s) == 0)

> >           sysgdbinit = relocated_sysgdbinit;

> > --

> > 2.23.0.rc1.153.gdeed80330f-goog

>

> Otherwise, LGTM.


Thanks.

Christian
Sergio Durigan Junior Aug. 21, 2019, 5:47 p.m. | #3
On Wednesday, August 21 2019, Christian Biesinger wrote:

> On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior

> <sergiodj@redhat.com> wrote:

>>

>> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

>>

>> > gdb/ChangeLog:

>> >

>> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

>> >

>> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.

>> >       (get_init_files): Update.

>>

>> I'm afraid you'll need a descriptive commit message :-).

>

> Changed to:

>

> 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

>

>         * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code

>         out of get_init_files.

>         (get_init_files): Update.

>

> (I guess the title of the commit message is not enough?)


Sorry, I should have been more clear.

The changelog entry was fine; I was referring to the actual commit
message (the text you write before the changelog).  I think a very
simple text should be enough in this case, since it's a refactorization.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Hannes Domani via Gdb-patches Aug. 21, 2019, 6:08 p.m. | #4
On Wed, Aug 21, 2019 at 12:47 PM Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
>

> On Wednesday, August 21 2019, Christian Biesinger wrote:

>

> > On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior

> > <sergiodj@redhat.com> wrote:

> >>

> >> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

> >>

> >> > gdb/ChangeLog:

> >> >

> >> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

> >> >

> >> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.

> >> >       (get_init_files): Update.

> >>

> >> I'm afraid you'll need a descriptive commit message :-).

> >

> > Changed to:

> >

> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

> >

> >         * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code

> >         out of get_init_files.

> >         (get_init_files): Update.

> >

> > (I guess the title of the commit message is not enough?)

>

> Sorry, I should have been more clear.

>

> The changelog entry was fine; I was referring to the actual commit

> message (the text you write before the changelog).  I think a very

> simple text should be enough in this case, since it's a refactorization.


Oh OK, changed to the following:
    Factor out the code to do the datadir-relocation for gdbinit

    This simplifies get_init_files and makes it possible to reuse
    this code in an upcoming patch for SYSTEM_GDBINIT_DIR.

    gdb/ChangeLog:

    2019-08-20  Christian Biesinger  <cbiesinger@google.com>

            * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code
            out of get_init_files.
            (get_init_files): Update.

(I assume it's fine not to resend the full patch, but let me know if
you want me to)

Christian
Sergio Durigan Junior Aug. 21, 2019, 6:10 p.m. | #5
On Wednesday, August 21 2019, Christian Biesinger wrote:

> On Wed, Aug 21, 2019 at 12:47 PM Sergio Durigan Junior

> <sergiodj@redhat.com> wrote:

>>

>> On Wednesday, August 21 2019, Christian Biesinger wrote:

>>

>> > On Wed, Aug 21, 2019 at 12:19 PM Sergio Durigan Junior

>> > <sergiodj@redhat.com> wrote:

>> >>

>> >> On Tuesday, August 20 2019, Christian Biesinger via gdb-patches wrote:

>> >>

>> >> > gdb/ChangeLog:

>> >> >

>> >> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

>> >> >

>> >> >       * main.c (relocate_gdbinit_path_maybe_in_datadir): New function.

>> >> >       (get_init_files): Update.

>> >>

>> >> I'm afraid you'll need a descriptive commit message :-).

>> >

>> > Changed to:

>> >

>> > 2019-08-20  Christian Biesinger  <cbiesinger@google.com>

>> >

>> >         * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code

>> >         out of get_init_files.

>> >         (get_init_files): Update.

>> >

>> > (I guess the title of the commit message is not enough?)

>>

>> Sorry, I should have been more clear.

>>

>> The changelog entry was fine; I was referring to the actual commit

>> message (the text you write before the changelog).  I think a very

>> simple text should be enough in this case, since it's a refactorization.

>

> Oh OK, changed to the following:

>     Factor out the code to do the datadir-relocation for gdbinit

>

>     This simplifies get_init_files and makes it possible to reuse

>     this code in an upcoming patch for SYSTEM_GDBINIT_DIR.

>

>     gdb/ChangeLog:

>

>     2019-08-20  Christian Biesinger  <cbiesinger@google.com>

>

>             * main.c (relocate_gdbinit_path_maybe_in_datadir): Factor this code

>             out of get_init_files.

>             (get_init_files): Update.

>

> (I assume it's fine not to resend the full patch, but let me know if

> you want me to)


That's great, thanks.

I can't approve it as I'm not a GM, but this LGTM.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Patch

diff --git a/gdb/main.c b/gdb/main.c
index b9e12589ab..a1d1904c9b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -191,6 +191,41 @@  relocate_gdb_directory (const char *initial, int flag)
   return dir;
 }
 
+static std::string relocate_gdbinit_path_maybe_in_datadir (std::string file)
+{
+  int datadir_len = strlen (GDB_DATADIR);
+
+  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
+     has been provided, search for SYSTEM_GDBINIT there.  */
+  if (gdb_datadir_provided
+      && datadir_len < file.length ()
+      && filename_ncmp (file.c_str (), GDB_DATADIR, datadir_len) == 0
+      && IS_DIR_SEPARATOR (file[datadir_len]))
+    {
+      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
+	 to gdb_datadir.  */
+
+      size_t start = datadir_len;
+      for (; IS_DIR_SEPARATOR (file[start]); ++start)
+	continue;
+      return std::string (gdb_datadir) + SLASH_STRING +
+	file.substr(start);
+    }
+  else
+    {
+      char *relocated = relocate_path (gdb_program_name,
+				       file.c_str(),
+				       SYSTEM_GDBINIT_RELOCATABLE);
+      if (relocated != nullptr)
+	{
+	  std::string retval(relocated);
+	  xfree (relocated);
+	  return retval;
+	}
+    }
+    return "";
+}
+
 /* Compute the locations of init files that GDB should source and
    return them in SYSTEM_GDBINIT, HOME_GDBINIT, LOCAL_GDBINIT.  If
    there is no system gdbinit (resp. home gdbinit and local gdbinit)
@@ -212,37 +247,8 @@  get_init_files (std::string *system_gdbinit,
 
       if (SYSTEM_GDBINIT[0])
 	{
-	  int datadir_len = strlen (GDB_DATADIR);
-	  int sys_gdbinit_len = strlen (SYSTEM_GDBINIT);
-	  std::string relocated_sysgdbinit;
-
-	  /* If SYSTEM_GDBINIT lives in data-directory, and data-directory
-	     has been provided, search for SYSTEM_GDBINIT there.  */
-	  if (gdb_datadir_provided
-	      && datadir_len < sys_gdbinit_len
-	      && filename_ncmp (SYSTEM_GDBINIT, GDB_DATADIR, datadir_len) == 0
-	      && IS_DIR_SEPARATOR (SYSTEM_GDBINIT[datadir_len]))
-	    {
-	      /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR
-		 to gdb_datadir.  */
-
-	      size_t start = datadir_len;
-	      for (; IS_DIR_SEPARATOR (SYSTEM_GDBINIT[start]); ++start)
-		continue;
-	      relocated_sysgdbinit = std::string (gdb_datadir) + SLASH_STRING +
-		&SYSTEM_GDBINIT[start];
-	    }
-	  else
-	    {
-	      char *relocated = relocate_path (gdb_program_name,
-					       SYSTEM_GDBINIT,
-					       SYSTEM_GDBINIT_RELOCATABLE);
-	      if (relocated != nullptr)
-	        {
-		  relocated_sysgdbinit = relocated;
-		  xfree (relocated);
-		}
-	    }
+	  std::string relocated_sysgdbinit =
+	    relocate_gdbinit_path_maybe_in_datadir (SYSTEM_GDBINIT);
 	  if (!relocated_sysgdbinit.empty () &&
 	      stat (relocated_sysgdbinit.c_str (), &s) == 0)
 	    sysgdbinit = relocated_sysgdbinit;