Remove gg_oauth_parameter_t

Review Request #516 — Created Feb. 15, 2021 and discarded

Information

pidgin/pidgin
default

Reviewers

Use variadic function gg_oauth_generate_request instead.

Compile.

Description From Last Updated

Since gg_oauth_parameter_set is only called in a block with gg_oauth_parameter_join right after it, I feel like a variadic gg_oath_parameter_generate(gboolean header, …

QuLogicQuLogic

Mark with G_GNUC_NULL_TERMINATED

QuLogicQuLogic

And then add NULL at the end.

QuLogicQuLogic

Ditto.

QuLogicQuLogic

A comment about how this works, ie returning a comma separated header value if header is true, or a url-encoded …

grimgrim

curious why g_string_append_printf wasn't used?

grimgrim

This might read a bit better and lead to less errors later as right now there's 3 checks for header …

grimgrim

We can but I don't see how g_string_truncate() can lead to memory errors.

qarkaiqarkai

I just realized this is to handel the trailing , or & from line 83. This can lead to memory …

grimgrim

I was thinking of something more like this for this function... static char * gg_oauth_generate_request(gboolean header, ...) { GString *res …

grimgrim
qarkai
QuLogic
  1. 
      
  2. libpurple/protocols/gg/oauth/oauth.c (Diff revision 2)
     
     
    Show all issues

    Since gg_oauth_parameter_set is only called in a block with gg_oauth_parameter_join right after it, I feel like a variadic gg_oath_parameter_generate(gboolean header, ...) might be simpler.

  3. 
      
qarkai
QuLogic
  1. 
      
  2. libpurple/protocols/gg/oauth/oauth.c (Diff revision 3)
     
     
    Show all issues

    Mark with G_GNUC_NULL_TERMINATED

  3. libpurple/protocols/gg/oauth/oauth.c (Diff revision 3)
     
     
    Show all issues

    And then add NULL at the end.

  4. libpurple/protocols/gg/oauth/oauth.c (Diff revision 3)
     
     
    Show all issues

    Ditto.

  5. 
      
qarkai
QuLogic
  1. Ship It!
  2. 
      
grim
  1. 
      
  2. libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)
     
     
    Show all issues

    A comment about how this works, ie returning a comma separated header value if header is true, or a url-encoded value would be nice.

  3. libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)
     
     
    Show all issues

    curious why g_string_append_printf wasn't used?

  4. libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)
     
     
    Show all issues

    This might read a bit better and lead to less errors later as right now there's 3 checks for header to be true.

    escaped = g_uri_escape_string(value, NULL, FALSE);
    if(header) {
        g_string_append_printf(res, "\"%s\",", escaped);
    } else {
        g_string_append_printf(res, "%s&", escaped);
    }
    g_free(escaped);
    
  5. libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)
     
     
    Show all issues

    I just realized this is to handel the trailing , or & from line 83. This can lead to memory errors. Can we instead have a boolean to say whether or not we've processed the first item, and if so, then prefix it.

    1. We can but I don't see how g_string_truncate() can lead to memory errors.

    2. While this shouldn't happen, it is currently 100% possible to call

      gg_oauth_generate_request(TRUE, NULL);
      

      After looking at the implementation of g_string_truncate this won't cause a memory error, but consumers of our own internal api's shouldn't need to know the implemention of another library's api.

      GString *
      g_string_truncate (GString *string,
                         gsize    len)
      {
        g_return_val_if_fail (string != NULL, NULL);
      
        string->len = MIN (len, string->len);
        string->str[string->len] = 0;
      
        return string;
      }
      
    3. Added checks around g_string_truncate.

  6. 
      
qarkai
  1. 
      
  2. libpurple/protocols/gg/oauth/oauth.c (Diff revision 4)
     
     
    Show all issues

    We can but I don't see how g_string_truncate() can lead to memory errors.

  3. 
      
qarkai
grim
  1. 
      
  2. libpurple/protocols/gg/oauth/oauth.c (Diff revision 5)
     
     
    Show all issues

    I was thinking of something more like this for this function...

    static char *
    gg_oauth_generate_request(gboolean header, ...) {
        GString *res = g_string_new(NULL);
        va_list params;
    
        if(header) {
            res = g_string_append(res, "OAuth ");
        }
    
        va_start(params, header);
        while((key = va_arg(params, const gchar *))) {
            const gchar *value = va_arg(params, const gchar *);
            gchar *escaped = g_uri_escape_string(value, NULL, FALSE);
    
            if(header) {
                g_string_append_printf(res, "\"%s\",", escaped);
            } else {
                g_string_append_printf(res, "%s&", escaped);
            }
    
            g_free(escaped);
        }
        va_end(params);
    
        if(res->len > 0 && res->str[res->len - 1] == separator) {
            /* remove trailing separator */
            res = g_string_truncate(res, res->len - 1);
        }
    
        return g_string_free(res, FALSE);
    }
    
    1. I forgot to drop the separator variable in the cleanup part which is still a problem if it's called with (FALSE, NULL) as well as (TRUE, NULL) where you'll end up with OAuth without a trailing space, but that's probably fine.

    2. This has redundant header comparison in each loop iteration. Also for me "\"%s\"," looks unclear and ugly.

    3. which is still a problem

      It's not a problem if separator is initialized. If it's called as (header, NULL) then res's last char is space which is not equal to separator being , or & so nothing truncated.

    4. Well, before there were 3 redundant header checks in the loop and that was my initial issue. Regardless the compiler will most likely optimize them out, but my concern is about the readability of the code.

      Adding additional variables isn't a good fix either as it's more stuff that someone has to remember while reading the code.

      As for the escaped quotes, that's just a fact of life in a format string. Making it a variable adds additional dependent format variables for which over complicates the entire thing. It's more error prone and then you have to understand what %s=%s%s%s%c means which is even more intimidating.

      Also I just noticed I missed the key part. My fault.

    5. Look here's the thing... I'm pushing to make this code base more readable. Additional variables and format strings like %s=%s%s%s%c is not what I consider more readable. I can either accept this as is and then change it myself, or we can change it here.

    6. Agree to disagree about readability. Feel free to change but I won't fix it, sorry.

    7. I fear this may have been taken personal, so to be clear, this is nothing personal, it is 100% about simplifying code. You're free to your opinion, but ultimately the decision comes to me. I'm sorry you feel so strongly about this, but I'm not going to change my position here and I hope this doesn't stop you from contributing in the future.

    8. I totally understand and it's ok to disagree. After all you are project's maintainer and last decision is yours.

  3. 
      
grim
Review request changed
Status:
Discarded
Change Summary:

superceded by https://reviews.imfreedom.org/r/525/