← Back to team overview

cloud-init-dev team mailing list archive

Re: [Merge] ~mgerdts/cloud-init:lp1667735 into cloud-init:master

 

I like it.
Good work.
I've pointed the c-i bot at it, so it will comment soon.

https://jenkins.ubuntu.com/server/job/cloud-init-ci/1005/

there are 2 tiny things inline.


Diff comments:

> diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py
> index 86bfa5d..5717dae 100644
> --- a/cloudinit/sources/DataSourceSmartOS.py
> +++ b/cloudinit/sources/DataSourceSmartOS.py
> @@ -360,6 +371,45 @@ class JoyentMetadataClient(object):
>          LOG.debug('Value "%s" found.', value)
>          return value
>  
> +    def _readline(self):
> +        """
> +           Reads a line a byte at a time until \n is encountered.  Returns an
> +           ascii string with the trailing newline removed.
> +
> +           If a timeout (per-byte) is set and it expires, a
> +           JoyentMetadataFetchException will be thrown.
> +        """
> +        response = bytearray()
> +        while True:
> +            try:
> +                byte = self.fp.read(1)
> +                if len(byte) == 0:
> +                    raise JoyentMetadataTimeoutException(
> +                        "Partial response: '%s'" % response.decode('ascii'))
> +                if ord(byte) == ord(b'\n'):

is ord needed here?
we read a byte. can't we just compare it to b'\n' directly?

> +                    return response.decode('ascii')
> +                response.extend([ord(byte)])

its probably not any sort of issue, but you might as well use .append(ord(byte))
as that cuts out the need for list creation. timeit says its faster, i'm sure its
not really important but might as well.

> +            except OSError as exc:
> +                if exc.errno == errno.EAGAIN:
> +                    raise JoyentMetadataTimeoutException(
> +                        "Partial response: '%s'" % response.decode('ascii'))
> +                raise
> +
> +    def _write(self, msg):
> +        self.fp.write(msg.encode('ascii'))
> +        self.fp.flush()
> +
> +    def _negotiate(self):
> +        LOG.debug('Negotiating protocol V2')
> +        self._write('NEGOTIATE V2\n')
> +        self.fp.flush()
> +        response = self._readline()
> +        LOG.debug('read "%s"' % response)
> +        if response != 'V2_OK':
> +            raise JoyentMetadataFetchException(
> +                'Invalid response "%s" to "NEGOTIATE V2"' % response)
> +        LOG.debug('Negotiation complete')
> +
>      def request(self, rtype, param=None):
>          request_id = '{0:08x}'.format(random.randint(0, 0xffffffff))
>          message_body = ' '.join((request_id, rtype,))


-- 
https://code.launchpad.net/~mgerdts/cloud-init/+git/cloud-init/+merge/343118
Your team cloud-init commiters is requested to review the proposed merge of ~mgerdts/cloud-init:lp1667735 into cloud-init:master.


References