[PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[OpenBMC Patches]

Fix parsing of Get IP address.
Dbus method returns additional information such as family and
gateway, but this IPMI option just requires the IP to be returned.

https://github.com/openbmc/phosphor-host-ipmid/pull/64

Adriana Kobylak (1):
  IPMI Get IP Support

 transporthandler.C | 75 ++++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 39 deletions(-)

-- 
2.7.1

[PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[OpenBMC Patches]

From: Adriana Kobylak <anoo@us.ibm.com>

Fix parsing of Get IP address.
Dbus method returns additional information such as family and
gateway, but this IPMI option just requires the IP to be returned.
---
 transporthandler.C | 75 ++++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 39 deletions(-)

diff --git a/transporthandler.C b/transporthandler.C
index df2986a..d43f1f9 100644
--- a/transporthandler.C
+++ b/transporthandler.C
@@ -215,9 +215,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
     unsigned char       prefixlen;
     unsigned char       scope;
     unsigned int        flags;
-    char                saddr [128];
-    char                gateway [128];
-    uint8_t             buf[11];
+    char               *saddr = NULL;
     int                 i = 0;
 
     printf("IPMI GET_LAN\n");
@@ -259,57 +257,56 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
     else if (reqptr->parameter == LAN_PARM_IP)
     {
         const char*         device             = "eth0";
+        uint8_t buf[5]; // Size of expected IPMI response msg
 
-        sd_bus_message *res = NULL;
-        sd_bus *bus1        = NULL;
-        sd_bus_error err    = SD_BUS_ERROR_NULL;
-
-        rc = sd_bus_open_system(&bus1);
-        if(rc < 0)
-        {
-            fprintf(stderr,"ERROR: Getting a SYSTEM bus hook\n");
+        r = sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetAddress4");
+        if (r < 0) {
+            fprintf(stderr, "Failed to add method object: %s\n", strerror(-r));
             return -1;
         }
-
-        rc = sd_bus_call_method(bus1,            // On the System Bus
-                                app,            // Service to contact
-                                obj,            // Object path 
-                                ifc,            // Interface name
-                                "GetAddress4",  // Method to be called
-                                &err,           // object to return error
-                                &res,           // Response message on success
-                                "s",         // input message (dev,ip,nm,gw)
-                                "eth0");
-        if(rc < 0)
-        {
-            fprintf(stderr, "Failed to Get IP on interface : %s\n", device);
+        r = sd_bus_message_append(m, "s", device);
+        if (r < 0) {
+            fprintf(stderr, "Failed to append message data: %s\n", strerror(-r));
             return -1;
         }
-
-        /* rc = sd_bus_message_read(res, "a(iyyus)s", ...); */
-        rc = sd_bus_message_enter_container (res, 'a', "(iyyus)");
+        r = sd_bus_call(bus, m, 0, &error, &reply);
+        if (r < 0) {
+            fprintf(stderr, "Failed to call method: %s\n", strerror(-r));
+            return -1;
+        }
+        rc = sd_bus_message_enter_container (reply, 'a', "(iyyus)");
         if(rc < 0)
         {
             fprintf(stderr, "Failed to parse response message:[%s]\n", strerror(-rc));
             return -1;
         }
-
-        while ((rc = sd_bus_message_read(res, "(iyyus)", &family, &prefixlen, &scope, &flags, &saddr)) > 0) {
-                printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6", prefixlen, scope, flags, saddr);
-        }
-
-        rc = sd_bus_message_read (res, "s", &gateway);
-        if(rc < 0)
+        rc = sd_bus_message_read(reply, "(iyyus)", &family, &prefixlen, &scope, &flags, &saddr);
+        if (rc < 0)
         {
-            fprintf(stderr, "Failed to parse gateway from response message:[%s]\n", strerror(-rc));
+            fprintf(stderr, "Failed to receive response: %s\n", strerror(-r));
             return -1;
         }
 
+        printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6", prefixlen, scope, flags, saddr);
+
         memcpy((void*)&buf[0], &current_revision, 1);
-        sscanf (saddr, "%c.%c.%c.%c", &buf[1], &buf[2], &buf[3], &buf[4]);
-        buf[5] = family;
-        buf[6] = prefixlen;
-        sscanf (gateway, "%c.%c.%c.%c", &buf[7], &buf[8], &buf[9], &buf[10]);
+
+        // Parse IP address
+        char *tokptr = NULL;
+        char* digit = strtok_r(saddr, ".", &tokptr);
+        if (digit == NULL)
+        {
+            fprintf(stderr, "Unexpected IP format: %s", saddr);
+            return IPMI_CC_RESPONSE_ERROR;
+        }
+        i = 0;
+        while (digit != NULL)
+        {
+            int resp_byte = strtoul(digit, NULL, 10);
+            memcpy((void*)&buf[i+1], &resp_byte, 1);
+            i++;
+            digit = strtok_r(NULL, ".", &tokptr);
+        }
 
         *data_len = sizeof(buf);
         memcpy(response, &buf, *data_len);
-- 
2.7.1

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Andrew Jeffery]

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

Hi Adriana,

I realise this has already been merged, but I feel like I should point
out some issues below for future improvements.

On Wed, 2016-02-10 at 15:40 -0600, OpenBMC Patches wrote:
> From: Adriana Kobylak <anoo@us.ibm.com>
> 
> Fix parsing of Get IP address.
> Dbus method returns additional information such as family and
> gateway, but this IPMI option just requires the IP to be returned.
> ---
>  transporthandler.C | 75 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 36 insertions(+), 39 deletions(-)
> 
> diff --git a/transporthandler.C b/transporthandler.C
> index df2986a..d43f1f9 100644
> --- a/transporthandler.C
> +++ b/transporthandler.C
> @@ -215,9 +215,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>      unsigned char       prefixlen;
>      unsigned char       scope;
>      unsigned int        flags;
> -    char                saddr [128];
> -    char                gateway [128];
> -    uint8_t             buf[11];
> +    char               *saddr = NULL;
>      int                 i = 0;
>  
>      printf("IPMI GET_LAN\n");
> @@ -259,57 +257,56 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>      else if (reqptr->parameter == LAN_PARM_IP)
>      {
>          const char*         device             = "eth0";
> +        uint8_t buf[5]; // Size of expected IPMI response msg

Might be best to use a constant and avoid the comment:

#define IPMI_IP_RESP_SIZE 5

>  
> -        sd_bus_message *res = NULL;
> -        sd_bus *bus1        = NULL;
> -        sd_bus_error err    = SD_BUS_ERROR_NULL;
> -
> -        rc = sd_bus_open_system(&bus1);
> -        if(rc < 0)
> -        {
> -            fprintf(stderr,"ERROR: Getting a SYSTEM bus hook\n");
> +        r = sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetAddress4");
> +        if (r < 0) {
> +            fprintf(stderr, "Failed to add method object: %s\n", strerror(-r));
>              return -1;
>          }
> -
> -        rc = sd_bus_call_method(bus1,            // On the System Bus
> -                                app,            // Service to contact
> -                                obj,            // Object path 
> -                                ifc,            // Interface name
> -                                "GetAddress4",  // Method to be called
> -                                &err,           // object to return error
> -                                &res,           // Response message on success
> -                                "s",         // input message (dev,ip,nm,gw)
> -                                "eth0");
> -        if(rc < 0)
> -        {
> -            fprintf(stderr, "Failed to Get IP on interface : %s\n", device);
> +        r = sd_bus_message_append(m, "s", device);
> +        if (r < 0) {
> +            fprintf(stderr, "Failed to append message data: %s\n", strerror(-r));
>              return -1;
>          }
> -
> -        /* rc = sd_bus_message_read(res, "a(iyyus)s", ...); */
> -        rc = sd_bus_message_enter_container (res, 'a', "(iyyus)");
> +        r = sd_bus_call(bus, m, 0, &error, &reply);
> +        if (r < 0) {
> +            fprintf(stderr, "Failed to call method: %s\n", strerror(-r));
> +            return -1;
> +        }
> +        rc = sd_bus_message_enter_container (reply, 'a', "(iyyus)");
>          if(rc < 0)
>          {
>              fprintf(stderr, "Failed to parse response message:[%s]\n", strerror(-rc));
>              return -1;
>          }
> -
> -        while ((rc = sd_bus_message_read(res, "(iyyus)", &family, &prefixlen, &scope, &flags, &saddr)) > 0) {
> -                printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6", prefixlen, scope, flags, saddr);
> -        }
> -
> -        rc = sd_bus_message_read (res, "s", &gateway);
> -        if(rc < 0)
> +        rc = sd_bus_message_read(reply, "(iyyus)", &family, &prefixlen, &scope, &flags, &saddr);
> +        if (rc < 0)
>          {
> -            fprintf(stderr, "Failed to parse gateway from response message:[%s]\n", strerror(-rc));
> +            fprintf(stderr, "Failed to receive response: %s\n", strerror(-r));
>              return -1;
>          }
>  
> +        printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6", prefixlen, scope, flags, saddr);
> +
>          memcpy((void*)&buf[0], ¤t_revision, 1);
> -        sscanf (saddr, "%c.%c.%c.%c", &buf[1], &buf[2], &buf[3], &buf[4]);
> -        buf[5] = family;
> -        buf[6] = prefixlen;
> -        sscanf (gateway, "%c.%c.%c.%c", &buf[7], &buf[8], &buf[9], &buf[10]);
> +
> +        // Parse IP address

Instead of hand-crafting this, why not use inet_pton()[1]? The binary
IP value is output in the s_addr member of struct in_addr. Also, we
need to do different things based on whether we have an IPv4 vs IPv6
address (assuming we care as there's detection of v4/v6 in the printf()
above), and with inet_pton() abstraction this is a change of a
parameter rather than a change of algorithm.

> +        char *tokptr = NULL;
> +        char* digit = strtok_r(saddr, ".", &tokptr);
> +        if (digit == NULL)
> +        {
> +            fprintf(stderr, "Unexpected IP format: %s", saddr);
> +            return IPMI_CC_RESPONSE_ERROR;
> +        }
> +        i = 0;
> +        while (digit != NULL)
> +        {
> +            int resp_byte = strtoul(digit, NULL, 10);

If we continue on this path (instead of using inet_pton() as suggested
above), with strtoul() you're assigning an unsigned value into a signed
type - it's a bit of nit but resp_byte should probably be unsigned.
Additionally, if you're using strtoul() you should to do error checking
as suggested in the NOTES section of the man page[2]. The example in
the strtol() man page[3] demonstrates that actually requires quite a
bit of effort.

> +            memcpy((void*)&buf[i+1], &resp_byte, 1);

I think really what you're trying to say here is that you expect the
value produced by strtoul() to be <= 255, and that you want to make use
of the parsed value. resp_byte should be tested for <= 255, as if the
parsed value is bigger it's a malformed IP. Malformed IPs should
probably result in an error being propagated to the caller.

Regardless, if we use inet_pton() instead we can just punt on the
problem of how to detect invalid IPs and deal with the error return
code (0, -1/errno) as necessary.

Additionally, the above approach locks the code to a little-endian
environment. I don't know that we care about being portable, but it's
easy to avoid the lock-in and the memcpy (assumes we've checked
resp_byte is in-range):

    buf[i+1] = (uint8_t) resp_byte;

And on that, it seems that we only use i in the context of the loop, in
the assignment into buf. Can we set i=1 prior to the loop, rather than
obfuscating the code unnecessarily with i+1?

Again, these last points are not relevant if we switch to inet_pton(),
though the s_addr value will still eventually need to be memcpy()ed
into the response buffer.

> +            i++;
> +            digit = strtok_r(NULL, ".", &tokptr);
> +        }
>  
>          *data_len = sizeof(buf);
>          memcpy(response, &buf, *data_len);

Andrew

[1] http://man7.org/linux/man-pages/man3/inet_pton.3.html
[2] http://man7.org/linux/man-pages/man3/strtoul.3.html#NOTES
[3] http://man7.org/linux/man-pages/man3/strtol.3.html#EXAMPLE

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Stewart Smith]

Andrew Jeffery <andrew@aj.id.au> writes:
> I realise this has already been merged, but I feel like I should point
> out some issues below for future improvements.

Perhaps unreviewed code shouldn't be merged. Especially considering that
there's been so many comments on so many patches in this project.

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Chris Austen]

[-- Attachment #1: Type: text/html, Size: 1585 bytes --]

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Andrew Jeffery]

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

On Thu, 2016-02-11 at 05:44 +0000, Chris Austen wrote:
> Open an issue to migrate to inet_pton.
>  

Okay, I've opened issue 65 for this:

https://github.com/openbmc/phosphor-host-ipmid/issues/65

Andrew

>  
> 
> Chris Austen
> POWER Systems Enablement Manager
> (512) 286-5184 (T/L: 363-5184)
>  
>  
> ----- Original message -----
> From: Stewart Smith <stewart@linux.vnet.ibm.com>
> Sent by: "openbmc" <
> openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org>
> To: andrew@aj.id.au, OpenBMC Patches <openbmc-patches@stwcx.xyz>, 
> openbmc@lists.ozlabs.org
> Cc:
> Subject: Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support
> Date: Wed, Feb 10, 2016 11:37 PM
>  
> Andrew Jeffery <andrew@aj.id.au> writes:
> > I realise this has already been merged, but I feel like I should
> point
> > out some issues below for future improvements.
> 
> Perhaps unreviewed code shouldn't be merged. Especially considering
> that
> there's been so many comments on so many patches in this project.
> 
> --
> Stewart Smith
> OPAL Architect, IBM.
> 
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Stewart Smith]

Chris Austen <austenc@us.ibm.com> writes:
> Open an issue to migrate to inet_pton.

Fixing such things in code review is a much better approach.

For the submitter, it helps get quick feedback on tehir code, and
promotes a culture of examining your own code closely for things before
submitting it.

For maintainers and those who hare going to have to debug the systems,
it means that it's *very* quick and cheap (in time) to point out odd
things.

Opening issues is approximately an order of magnitude slower.

I can hit reply, type ten words and hit send in maybe a few seconds. For
an issue, you're adding a couple of seconds in network round trip to
browse to the project, go to issues, click create issue, then type a
whole description of the problem because you don't have the context of
what you're trying to reply to, and then manage the issue by adding
tags, target releases, closing it when something is finally merged (and
having the submitter have to open another pull request and keep track of
it)... Urgh.

Good code review provides a positive feedback loop of improvement to
code with a positive reinforcement at the end of "patch merged!"
Contrast this to the negative re-inforcement of filing bugs, of which
there is little to no direct motivation for the code author to go and fix.

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Andrew Jeffery]

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

On Fri, 2016-02-12 at 08:15 +1100, Stewart Smith wrote:
> Chris Austen <austenc@us.ibm.com> writes:
> > Open an issue to migrate to inet_pton.
> 
> Fixing such things in code review is a much better approach.
> 
> For the submitter, it helps get quick feedback on tehir code, and
> promotes a culture of examining your own code closely for things before
> submitting it.
> 
> For maintainers and those who hare going to have to debug the systems,
> it means that it's *very* quick and cheap (in time) to point out odd
> things.
> 
> Opening issues is approximately an order of magnitude slower.

For transparency, I sent the email despite knowing the code had been
merged at the point I hit send because the timeline went along the
lines of: Open my laptop for the day, start reading email, come across
the patch, find some areas that I think we can improve on, write 90% of
the reply, head to github to check a small detail, discover the patch
has already been merged.

Since I'd spent some time composing the review I didn't want to throw
it away or spend further time to massaging it into a github issue with
the necessary context, so I reworded the first paragraph to reflect the
fact that it was already merged and hit send. This dovetails into
Stewart's point below, primarily the one about github issues lacking
the code context for the comments (without some manual effort). To be
fair as it stands the github issue I created doesn't (yet) contain any
of the context or comments provided in the email review, but at least
that's available through the list archives (and can be easily linked in
a comment).

Maybe this should be a separate thread, but anyway: I find the mailing
-list/github split a little awkward - is there a way we can improve
this? Use one xor the other?

Andrew

> 
> I can hit reply, type ten words and hit send in maybe a few seconds. For
> an issue, you're adding a couple of seconds in network round trip to
> browse to the project, go to issues, click create issue, then type a
> whole description of the problem because you don't have the context of
> what you're trying to reply to, and then manage the issue by adding
> tags, target releases, closing it when something is finally merged (and
> having the submitter have to open another pull request and keep track of
> it)... Urgh.
> 
> Good code review provides a positive feedback loop of improvement to
> code with a positive reinforcement at the end of "patch merged!"
> Contrast this to the negative re-inforcement of filing bugs, of which
> there is little to no direct motivation for the code author to go and fix.
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Chris Austen]

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

Would it make sense to integrate gerrit?  
   
    I love the review I'm all for it.  I asked for the issue because I knew it was merged. No issues at all with comments. 
    
    I've work a project with gerrit and it seems that would resolve the issue with losing context
  
  Sent from my iPhone using IBM Verse
  
  On Feb 11, 2016, 5:09:41 PM, "Andrew Jeffery" <andrew@aj.id.au> wrote:
  
     On Fri, 2016-02-12 at 08:15 +1100, Stewart Smith wrote:
   > Chris Austen  writes:
   > > Open an issue to migrate to inet_pton.
   > 
   > Fixing such things in code review is a much better approach.
   > 
   > For the submitter, it helps get quick feedback on tehir code, and
   > promotes a culture of examining your own code closely for things before
   > submitting it.
   > 
   > For maintainers and those who hare going to have to debug the systems,
   > it means that it's *very* quick and cheap (in time) to point out odd
   > things.
   > 
   > Opening issues is approximately an order of magnitude slower.
   For transparency, I sent the email despite knowing the code had been
   merged at the point I hit send because the timeline went along the
   lines of: Open my laptop for the day, start reading email, come across
   the patch, find some areas that I think we can improve on, write 90% of
   the reply, head to github to check a small detail, discover the patch
   has already been merged.
   Since I'd spent some time composing the review I didn't want to throw
   it away or spend further time to massaging it into a github issue with
   the necessary context, so I reworded the first paragraph to reflect the
   fact that it was already merged and hit send. This dovetails into
   Stewart's point below, primarily the one about github issues lacking
   the code context for the comments (without some manual effort). To be
   fair as it stands the github issue I created doesn't (yet) contain any
   of the context or comments provided in the email review, but at least
   that's available through the list archives (and can be easily linked in
   a comment).
   Maybe this should be a separate thread, but anyway: I find the mailing
   -list/github split a little awkward - is there a way we can improve
   this? Use one xor the other?
   Andrew
   > 
   > I can hit reply, type ten words and hit send in maybe a few seconds. For
   > an issue, you're adding a couple of seconds in network round trip to
   > browse to the project, go to issues, click create issue, then type a
   > whole description of the problem because you don't have the context of
   > what you're trying to reply to, and then manage the issue by adding
   > tags, target releases, closing it when something is finally merged (and
   > having the submitter have to open another pull request and keep track of
   > it)... Urgh.
   > 
   > Good code review provides a positive feedback loop of improvement to
   > code with a positive reinforcement at the end of "patch merged!"
   > Contrast this to the negative re-inforcement of filing bugs, of which
   > there is little to no direct motivation for the code author to go and fix.
   >

[-- Attachment #2: Type: text/html, Size: 5839 bytes --]

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Jeremy Kerr]

Hi all,

> Maybe this should be a separate thread, but anyway: I find the mailing
> -list/github split a little awkward - is there a way we can improve
> this? Use one xor the other?

I'd love to move to single option too - to keep to our
open-source-project roots, my vote is for using the mailing list.

However, the roadblock here is that our developers would need to be able
to email patches in an acceptable format, and I don't think many of our
contributors have to tools and/or infrastructure to do that at the
moment. If that were to be addressed, I think mailing-list based
workflow would be a great benefit to the project.

Regards,


Jeremy

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Stewart Smith]

Jeremy Kerr <jk@ozlabs.org> writes:
>> Maybe this should be a separate thread, but anyway: I find the mailing
>> -list/github split a little awkward - is there a way we can improve
>> this? Use one xor the other?
>
> I'd love to move to single option too - to keep to our
> open-source-project roots, my vote is for using the mailing list.
>
> However, the roadblock here is that our developers would need to be able
> to email patches in an acceptable format, and I don't think many of our
> contributors have to tools and/or infrastructure to do that at the
> moment. If that were to be addressed, I think mailing-list based
> workflow would be a great benefit to the project.

LTC IMAP accounts should be available to everyone - if they put
OpenBMC/OpenPower in their BluePages profile, the LTC IMAP admins should
approve it.

You can, of course, not use IBM Verse corporate email, because it's
broken and when I file bugs they refuse to fix them or string us along
for (literally) years.

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Stewart Smith]

Chris Austen <austenc@us.ibm.com> writes:
> Would it make sense to integrate gerrit?

Do you have a place to run it publicly on the internet?

This is the sticking point in any discussion on self-hosting such tools.

review on the list does work, as does patchwork, and it requires no
special tools for people doing review that require a working internet
connection. It would also be consistent with similar open source
projects (e.g. skiboot, linux, buildroot, petitboot) so we wouldn't end
up with too much excessive variation on developer workflow.

While I do understand that IBM does not like to provide people with
standards compliant internet email... this shouldn't be a driving
factor on process, it should be a driving factor for people to get
actual internet email accounts though.

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH phosphor-host-ipmid v2] IPMI Get IP Support

[Andrew Jeffery]

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

On Fri, 2016-02-12 at 03:12 +0000, Chris Austen wrote:
> Would it make sense to integrate gerrit?  
> 
> I love the review I'm all for it.  I asked for the issue because I
> knew it was merged. No issues at all with comments. 
> 
> I've work a project with gerrit and it seems that would resolve the
> issue with losing context

In a past life I (haphazardly) maintained (and used) a Gerrit instance
for several years. It has its pros and cons, but probably the bigger
issue as Stewart points out would be fracturing the development process
with respect to other projects in the OpenPOWER stack (along with the
problems of hosting/sysadmin/system integration).

It's convenient to have all comments visible against a line rather than
spread over multiple reviewers' emails, but from a feature perspective
a fairly large drawback was Gerrit's handling of patch series. Up until
recently management of a series could be half-heartedly done through
'topics', but the patches still needed to be submitted individually
through the web UI or by a combination of web UI and git operations.
There was a plan to introduce a feature to atomically merge all patches
in a topic, but I haven't checked on the state of that recently.
There's also some tedium involved when patches are split or squashed,
as the older revisions can often (confusingly) hang around in the web
UI until they are manually abandoned. A simple 'v2' does away with
those problems on a mailing list or github pull-req. But maybe these
are usability issues that we can work around/cope with? Overall I don't
think Gerrit would be my choice in this context.

Using the mailing list with patchwork and snowpatch (a patchwork CI
-related tool brewing at ozlabs) might give us github-levels of CI
testing infra integration without the need for github? That would just
leave sorting out email for non-LTC IBM people...

Andrew

> 
> Sent from my iPhone using IBM Verse
> 
> On Feb 11, 2016, 5:09:41 PM, "Andrew Jeffery" <andrew@aj.id.au> wrote:
> On Fri, 2016-02-12 at 08:15 +1100, Stewart Smith wrote:
> > Chris Austen  writes:
> > > Open an issue to migrate to inet_pton.
> > 
> > Fixing such things in code review is a much better approach.
> > 
> > For the submitter, it helps get quick feedback on tehir code, and
> > promotes a culture of examining your own code closely for things before
> > submitting it.
> > 
> > For maintainers and those who hare going to have to debug the systems,
> > it means that it's *very* quick and cheap (in time) to point out odd
> > things.
> > 
> > Opening issues is approximately an order of magnitude slower.
> For transparency, I sent the email despite knowing the code had been
> merged at the point I hit send because the timeline went along the
> lines of: Open my laptop for the day, start reading email, come across
> the patch, find some areas that I think we can improve on, write 90% of
> the reply, head to github to check a small detail, discover the patch
> has already been merged.
> Since I'd spent some time composing the review I didn't want to throw
> it away or spend further time to massaging it into a github issue with
> the necessary context, so I reworded the first paragraph to reflect the
> fact that it was already merged and hit send. This dovetails into
> Stewart's point below, primarily the one about github issues lacking
> the code context for the comments (without some manual effort). To be
> fair as it stands the github issue I created doesn't (yet) contain any
> of the context or comments provided in the email review, but at least
> that's available through the list archives (and can be easily linked in
> a comment).
> Maybe this should be a separate thread, but anyway: I find the mailing
> -list/github split a little awkward - is there a way we can improve
> this? Use one xor the other?
> Andrew
> > 
> > I can hit reply, type ten words and hit send in maybe a few seconds. For
> > an issue, you're adding a couple of seconds in network round trip to
> > browse to the project, go to issues, click create issue, then type a
> > whole description of the problem because you don't have the context of
> > what you're trying to reply to, and then manage the issue by adding
> > tags, target releases, closing it when something is finally merged (and
> > having the submitter have to open another pull request and keep track of
> > it)... Urgh.
> > 
> > Good code review provides a positive feedback loop of improvement to
> > code with a positive reinforcement at the end of "patch merged!"
> > Contrast this to the negative re-inforcement of filing bugs, of which
> > there is little to no direct motivation for the code author to go and fix.
> > 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]