[U-Boot] [RFC PATCH v2 07/20] net: fastboot: Merge AOSP UDP fastboot

[Alex Kiernan]

On Tue, May 8, 2018 at 4:24 PM Joe Hershberger <joe.hershberger at ni.com>
wrote:

> On Tue, May 8, 2018 at 4:11 AM, Alex Kiernan <alex.kiernan at gmail.com>
wrote:
> > On Thu, May 3, 2018 at 9:39 PM Joe Hershberger <joe.hershberger at ni.com>
> > wrote:
> >
> >> On Mon, Apr 30, 2018 at 3:32 AM, Alex Kiernan <alex.kiernan at gmail.com>
> > wrote:
> >
> > <snip>
> >
> >> > +/**
> >> > + * Constructs and sends a packet in response to received fastboot
> > packet
> >> > + *
> >> > + * @param fb_header            Header for response packet
> >> > + * @param fastboot_data        Pointer to received fastboot data
> >> > + * @param fastboot_data_len    Length of received fastboot data
> >> > + * @param retransmit           Nonzero if sending last sent packet
> >> > + */
> >> > +static void fastboot_send(struct fastboot_header fb_header, char
> > *fastboot_data,
> >> > +                         unsigned int fastboot_data_len, uchar
> > retransmit)
> >> > +{
> >> > +       uchar *packet;
> >> > +       uchar *packet_base;
> >> > +       int len = 0;
> >> > +       const char *error_msg = "An error occurred.";
> >> > +       short tmp;
> >> > +       struct fastboot_header fb_response_header = fb_header;
> >> > +       char response[FASTBOOT_RESPONSE_LEN] = {0};
> >> > +       /*
> >> > +        *      We will always be sending some sort of packet, so
> >> > +        *      cobble together the packet headers now.
> >> > +        */
> >> > +       packet = net_tx_packet + net_eth_hdr_size() +
IP_UDP_HDR_SIZE;
> >> > +       packet_base = packet;
> >> > +
> >> > +       /* Resend last packet */
> >> > +       if (retransmit) {
> >> > +               memcpy(packet, last_packet, last_packet_len);
> >> > +               net_send_udp_packet(net_server_ethaddr,
> > fastboot_remote_ip,
> >> > +                                   fastboot_remote_port,
> > fastboot_our_port,
> >> > +                                   last_packet_len);
> >> > +               return;
> >> > +       }
> >> > +
> >> > +       fb_response_header.seq = htons(fb_response_header.seq);
> >> > +       memcpy(packet, &fb_response_header,
sizeof(fb_response_header));
> >> > +       packet += sizeof(fb_response_header);
> >> > +
> >> > +       switch (fb_header.id) {
> >> > +       case FASTBOOT_QUERY:
> >> > +               tmp = htons(fb_sequence_number);
> >> > +               memcpy(packet, &tmp, sizeof(tmp));
> >> > +               packet += sizeof(tmp);
> >> > +               break;
> >> > +       case FASTBOOT_INIT:
> >> > +               tmp = htons(fb_udp_version);
> >> > +               memcpy(packet, &tmp, sizeof(tmp));
> >> > +               packet += sizeof(tmp);
> >> > +               tmp = htons(fb_packet_size);
> >> > +               memcpy(packet, &tmp, sizeof(tmp));
> >> > +               packet += sizeof(tmp);
> >> > +               break;
> >> > +       case FASTBOOT_ERROR:
> >> > +               memcpy(packet, error_msg, strlen(error_msg));
> >> > +               packet += strlen(error_msg);
> >> > +               break;
> >> > +       case FASTBOOT_FASTBOOT:
> >> > +               if (!cmd_string) {
> >> > +                       /* Parse command and send ack */
> >> > +                       cmd_parameter = fastboot_data;
> >
> >> This seems unnecessary. There are only 4 cases handled, and of them
> >> only download seems to be a command that happens more than once. And
> >> in download, the first past through this parameter is saved internally
> >> as bytes_expected.
> >
> >> > +                       cmd_string = strsep(&cmd_parameter, ":");
> >> > +                       cmd_string = strdup(cmd_string);
> >> > +                       if (cmd_parameter)
> >> > +                               cmd_parameter =
strdup(cmd_parameter);
> >> > +               } else if (!strcmp("getvar", cmd_string)) {
> >> > +                       fb_getvar(response);
> >
> >> Seems like you should simply pass the "fastboot_data" as a parameter
> >> here rather than using a global.
> >
> > I'm completely pulling this apart in a later patch. I wonder if I should
> > really be folding some of these back into this - I was trying to
maintain a
> > clear line to the AOSP code which this came from.

> What value do we get from that? Is there a merge technique you are
> trying to enable to make pulling in future AOSP changes easier? Are
> you trying to isolate your changes from AOSP for attribution purposes?

I was trying to maintain the attribution line, but given the level of
change I'm struggling to achieve that in what's an increasingly complex
chain of patches which end up interacting too much.

Given Alex and Jocelyn are engaged, I'm not sure I see the value in
continuing trying to do this.

> So far I'm of the opinion that the fixes to the existing code should
> simply be there in the originating patch. Feel free to convince me
> otherwise.

I'll fail to convince you as I'm increasingly unconvinced myself... I'm
spending more time rebasing patches than actually separating the code into
something which can handle both UDP and USB transports.

--
Alex Kiernan