Uploaded image for project: 'Derby'
  1. Derby
  2. DERBY-6997

Confusing code in NetworkServerControlImpl could be improved

    XMLWordPrintableJSON

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 10.14.2.0
    • Fix Version/s: None
    • Component/s: Network Server
    • Labels:
      None

      Description

      As was discussed on the derby-dev list here: http://mail-archives.apache.org/mod_mbox/db-derby-dev/201802.mbox/%3CCANi-yg8ru8BLNVxW0v6TT_ndPa8Y7_MD6zpgjiyQ6MQoT4Ca9g%40mail.gmail.com%3E

      it seems like there is some confusing shifting-and-masking code in NetworkServerControlImpl that could be improved:

          private void writeShort(int value) throws Exception
          {
              try {
                  commandOs.writeByte((byte)((value & 0xf0) >> 8 ));
                  commandOs.writeByte((byte)(value & 0x0f));
              }
              catch (IOException e)
              {
                  clientSocketError(e);
              }
          }
      

      I'm not quite sure what this code is doing.

      It seems to be mixing some combination of 8-bit shifting
      and 4-bit value masking.

      I think it might actually be losing/destroying 4 bits of
      the 16-bit value (the high 4 bytes in the low byte of
      the short).

      For example if you call writeShort(23), it emits 0x00, 0x07,
      when I think it should emit 0x00,0x17.

      I suspect the code should read:

          private void writeShort(int value) throws Exception
          {
              try {
                  commandOs.writeByte((byte)((value & 0xff00) >> 8 ));
                  commandOs.writeByte((byte)(value & 0x00ff));
              }
              catch (IOException e)
              {
                  clientSocketError(e);
              }
          }
      

      or perhaps

          private void writeShort(int value) throws Exception
          {
              try {
                  commandOs.writeByte((byte)((value >> 8 ));
                  commandOs.writeByte((byte)value);
              }
              catch (IOException e)
              {
                  clientSocketError(e);
              }
          }
      

      or some such, but really I don't understand it at all.

      And, very close to this code is a very similar method:

          private void writeByte(int value) throws Exception
          {
              try {
                  commandOs.writeByte((byte)(value & 0x0f));
              }
              catch (IOException e)
              {
                  clientSocketError(e);
              }
          }
      

      which I think might be damaged for any value in the
      range 17-255? It seems like it should probably be:

          private void writeByte(int value) throws Exception
          {
              try {
                  commandOs.writeByte((byte)(value & 0xff));
              }
              catch (IOException e)
              {
                  clientSocketError(e);
              }
          }
      

      or maybe just:

          private void writeByte(int value) throws Exception
          {
              try {
                  commandOs.writeByte((byte)value);
              }
              catch (IOException e)
              {
                  clientSocketError(e);
              }
          }
      

        Attachments

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              bryanpendleton Bryan Pendleton
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated: