[Merge] lp:~sergiusens/nuntium/encode_decode_tests into lp:nuntium

Wed Jul 2 08:37:09 UTC 2014

Review: Needs Fixing

I know if migh tbe annoying, but as a favour to the next nuntium developers I would add comments on what &0x7f and 0x80 represent.

Diff comments:

> === modified file 'mms/decoder.go'
> --- mms/decoder.go	2014-07-01 12:34:39 +0000
> +++ mms/decoder.go	2014-07-01 12:34:39 +0000
> @@ -82,15 +82,20 @@
>  
>  func (dec *MMSDecoder) ReadLength(reflectedPdu *reflect.Value) (length uint64, err error) {
>  	switch {
> -	case dec.Data[dec.Offset+1] < SHORT_LENGTH_MAX:
> +	case dec.Data[dec.Offset+1]&0x7f < SHORT_LENGTH_MAX:

I would add a comment explaining what is &0x7f and why is there.

>  		l, err := dec.ReadShortInteger(nil, "")
>  		v := uint64(l)
> -		reflectedPdu.FieldByName("Length").SetUint(v)
> +		if reflectedPdu != nil {
> +			reflectedPdu.FieldByName("Length").SetUint(v)
> +		}
>  		return v, err
>  	case dec.Data[dec.Offset+1] == LENGTH_QUOTE:
>  		dec.Offset++
> -		l, err := dec.ReadUintVar(reflectedPdu, "Length")
> -		return l, err
> +		var hdr string
> +		if reflectedPdu != nil {
> +			hdr = "Length"
> +		}
> +		return dec.ReadUintVar(reflectedPdu, hdr)
>  	}
>  	return 0, fmt.Errorf("Unhandled length %#x @%d", dec.Data[dec.Offset+1], dec.Offset)
>  }
> @@ -257,14 +262,17 @@
>  func (dec *MMSDecoder) ReadLongInteger(reflectedPdu *reflect.Value, hdr string) (uint64, error) {
>  	dec.Offset++
>  	size := int(dec.Data[dec.Offset])
> +	if size > SHORT_LENGTH_MAX {
> +		return 0, fmt.Errorf("cannot encode long integer, lenght was %d but expected %d", size, SHORT_LENGTH_MAX)
> +	}
>  	dec.Offset++
> +	end := dec.Offset + size
>  	var v uint64
> -	endOffset := dec.Offset + size - 1
> -	v = v << 8
> -	for ; dec.Offset < endOffset; dec.Offset++ {
> +	for ; dec.Offset < end; dec.Offset++ {
> +		v = v << 8
>  		v |= uint64(dec.Data[dec.Offset])
> -		v = v << 8
>  	}
> +	dec.Offset--
>  	if hdr != "" {
>  		reflectedPdu.FieldByName(hdr).SetUint(uint64(v))
>  		dec.log = dec.log + fmt.Sprintf("Setting %s to %d\n", hdr, v)
> 
> === added file 'mms/encode_decode_test.go'
> --- mms/encode_decode_test.go	1970-01-01 00:00:00 +0000
> +++ mms/encode_decode_test.go	2014-07-01 12:34:39 +0000
> @@ -0,0 +1,110 @@
> +/*
> + * Copyright 2014 Canonical Ltd.
> + *
> + * Authors:
> + * Sergio Schvezov: sergio.schvezov at cannical.com
> + *
> + * This file is part of mms.
> + *
> + * mms is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 3.
> + *
> + * mms is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +package mms
> +
> +import (
> +	"bytes"
> +
> +	. "launchpad.net/gocheck"
> +)
> +
> +type EncodeDecodeTestSuite struct {
> +	bytes *bytes.Buffer
> +	enc   *MMSEncoder
> +	dec   *MMSDecoder
> +}
> +
> +var _ = Suite(&EncodeDecodeTestSuite{})
> +
> +func (s *EncodeDecodeTestSuite) SetUpTest(c *C) {
> +	s.bytes = new(bytes.Buffer)
> +	s.enc = NewEncoder(s.bytes)
> +	c.Assert(s.enc.writeByte(0), IsNil)
> +}
> +
> +func (s *EncodeDecodeTestSuite) TestString(c *C) {
> +	testStr := "'Hello World!"
> +	c.Assert(s.enc.writeString(testStr), IsNil)
> +	s.dec = NewDecoder(s.bytes.Bytes())
> +
> +	str, err := s.dec.ReadString(nil, "")
> +	c.Assert(err, IsNil)
> +	c.Assert(str, Equals, testStr)
> +}
> +
> +func (s *EncodeDecodeTestSuite) TestByte(c *C) {
> +	testBytes := []byte{0, 0x79, 0x80, 0x81}
> +	for i := range testBytes {
> +		c.Assert(s.enc.writeByte(testBytes[i]), IsNil)
> +	}
> +	bytes := s.bytes.Bytes()
> +	s.dec = NewDecoder(bytes)
> +	for i := range testBytes {
> +		b, err := s.dec.ReadByte(nil, "")
> +		c.Assert(err, IsNil)
> +		c.Assert(b, Equals, testBytes[i], Commentf("From testBytes[%d] and encoded bytes: %#x", i, bytes))
> +	}
> +}
> +
> +func (s *EncodeDecodeTestSuite) TestInteger(c *C) {
> +	// 128 bounds short and long integers
> +	testInts := []uint64{512, 100, 127, 128, 129, 255, 256, 511, 3000}
> +	for i := range testInts {
> +		c.Assert(s.enc.writeInteger(testInts[i]), IsNil)
> +	}
> +	bytes := s.bytes.Bytes()
> +	s.dec = NewDecoder(bytes)
> +	for i := range testInts {
> +		integer, err := s.dec.ReadInteger(nil, "")
> +		c.Assert(err, IsNil)
> +		c.Check(integer, Equals, testInts[i], Commentf("%d != %d with encoded bytes starting at %d: %d", integer, testInts[i], i, bytes))
> +	}
> +}
> +
> +func (s *EncodeDecodeTestSuite) TestUintVar(c *C) {
> +	testInts := []uint64{127, 512, 255, 256, 3000}
> +	for i := range testInts {
> +		c.Assert(s.enc.writeUintVar(testInts[i]), IsNil)
> +	}
> +	bytes := s.bytes.Bytes()
> +	s.dec = NewDecoder(bytes)
> +	for i := range testInts {
> +		integer, err := s.dec.ReadUintVar(nil, "")
> +		c.Assert(err, IsNil)
> +		c.Check(integer, Equals, testInts[i], Commentf("%d != %d with encoded bytes starting at %d: %d", integer, testInts[i], i, bytes))
> +	}
> +}
> +
> +func (s *EncodeDecodeTestSuite) TestLength(c *C) {
> +	// >= 30 requires encoding with length quote
> +	testLengths := []uint64{10, 1, 29, 30, 500}
> +	for i := range testLengths {
> +		c.Assert(s.enc.writeLength(testLengths[i]), IsNil)
> +	}
> +	bytes := s.bytes.Bytes()
> +	s.dec = NewDecoder(bytes)
> +	for i := range testLengths {
> +		integer, err := s.dec.ReadLength(nil)
> +		c.Assert(err, IsNil, Commentf("%d != %d with encoded bytes starting at %d: %d", integer, testLengths[i], i, bytes))
> +		c.Check(integer, Equals, testLengths[i], Commentf("%d != %d with encoded bytes starting at %d: %d", integer, testLengths[i], i, bytes))
> +	}
> +}
> 
> === modified file 'mms/encoder.go'
> --- mms/encoder.go	2014-07-01 12:34:39 +0000
> +++ mms/encoder.go	2014-07-01 12:34:39 +0000
> @@ -281,41 +281,38 @@
>  }
>  
>  func (enc *MMSEncoder) writeLongInteger(i uint64) error {
> -	long := i
>  	var encodedLong []byte
> -	for long > 0 {
> -		b := byte(0xff & long)
> -		encodedLong = append(encodedLong, b)
> -		long = long >> 8
> +	for i > 0 {
> +		b := byte(0xff & i)
> +		encodedLong = append([]byte{b}, encodedLong...)
> +		i = i >> 8
>  	}
>  
>  	encLength := uint64(len(encodedLong))
>  	if encLength > SHORT_LENGTH_MAX {
>  		return fmt.Errorf("cannot encode long integer, lenght was %d but expected %d", encLength, SHORT_LENGTH_MAX)
>  	}
> -	if err := enc.writeShortInteger(encLength); err != nil {
> +	if err := enc.writeByte(byte(encLength)); err != nil {
>  		return err
>  	}
>  	return enc.writeBytes(encodedLong, len(encodedLong))
>  }
>  
>  func (enc *MMSEncoder) writeInteger(i uint64) error {
> -	if i&0x80 < 0x80 {
> +	if i < 0x80 {

Same as with the previous comment, can you please add a small comment of what is 0x80.

>  		return enc.writeShortInteger(i)
>  	} else {
> -		fmt.Sprintf("Writing long int %d\n", i)
>  		return enc.writeLongInteger(i)
>  	}
>  	return nil
>  }
>  
>  func (enc *MMSEncoder) writeUintVar(v uint64) error {
> -	b := byte(v)
> -	uintVar := []byte{b & 0x7f}
> -	b = b >> 7
> -	for b > 0 {
> -		uintVar = append([]byte{b & 0x7f}, uintVar...)
> -		b = b >> 7
> +	uintVar := []byte{byte(v & 0x7f)}

Same with the previous ones :)

> +	v = v >> 7
> +	for v > 0 {
> +		uintVar = append([]byte{byte(0x80 | (v & 0x7f))}, uintVar...)
> +		v = v >> 7
>  	}
>  	return enc.writeBytes(uintVar, len(uintVar))
>  }
> 

-- 
https://code.launchpad.net/~sergiusens/nuntium/encode_decode_tests/+merge/225152
Your team Ubuntu Phablet Team is subscribed to branch lp:nuntium.