Merge ~chad.smith/cloud-init:uninitialized-variables into cloud-init:master

Proposed by Chad Smith
Status: Merged
Approved by: Scott Moser
Approved revision: efab669ff3d9c0089b0686c03234093b629bb1c8
Merge reported by: Chad Smith
Merged at revision: 1d8c327139a8c291eeb244ee1a6a8badd83e9e72
Proposed branch: ~chad.smith/cloud-init:uninitialized-variables
Merge into: cloud-init:master
Diff against target: 146 lines (+33/-16)
6 files modified
cloudinit/cmd/status.py (+5/-2)
cloudinit/cmd/tests/test_status.py (+18/-3)
cloudinit/config/cc_power_state_change.py (+1/-0)
cloudinit/config/cc_rh_subscription.py (+2/-3)
cloudinit/distros/freebsd.py (+3/-8)
cloudinit/util.py (+4/-0)
Reviewer Review Type Date Requested Status
Scott Moser Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+336453@code.launchpad.net

Commit message

Fix potential cases of uninitialized variables.

While addressing undeclared variable in 'cloud-init status', I also fixed
the errors raised by automated code reviews against cloud-init master at
https://lgtm.com/projects/g/cloud-init/cloud-init/alerts

The following items are addressed:

 * Fix 'cloud-init status':
    * Only report 'running' state when any stage in
      /run/cloud-init/status.json has a start time but no finished time.
      Default start time to 0 if null.
    * undeclared variable 'reason' now reports 'Cloud-init enabled by
      systemd cloud-init-generator' when systemd enables cloud-init

 * cc_rh_subscription.py util.subp return values aren't set during if an
   exception is raised, use ProcessExecution as e instead.

 * distros/freebsd.py:
   * Drop repetitive looping over ipv4 and ipv6 nic lists.
   * Initialize bsddev to 'NOTFOUND' in the event that no devs are
     discovered
   * declare nics_with_addresses = set() in broader scope outside
     check_downable conditional

 * cloudinit/util.py: Raise TypeError if mtype parameter isn't string,
   iterable or None.

LP: #1744796

Description of the change

See commit message

To post a comment you must log in.
Revision history for this message
Chad Smith (chad.smith) wrote :
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

FAILED: Continuous integration, rev:1669768f15889a2224735cae8ddc951087b96a2e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/713/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/713/rebuild

review: Needs Fixing (continuous-integration)
e7bcfc5... by Chad Smith

fix uninitialized local variable alerts as raised by lgtm

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:e7bcfc5e193ad84c7263b1ac6ff351291d35aab1
https://jenkins.ubuntu.com/server/job/cloud-init-ci/734/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/734/rebuild

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) :
efab669... by Chad Smith

Address scott's review comments:

- Use set declaration to coalesce get_ipv4|ipv6 devices in freebsd
- whitespace docstring unit test fix
- drop trailing ] from log message
- non-zero value for execmd in cc_power_state_change

Revision history for this message
Chad Smith (chad.smith) :
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:efab669ff3d9c0089b0686c03234093b629bb1c8
https://jenkins.ubuntu.com/server/job/cloud-init-ci/739/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/739/rebuild

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

thanks.
lgtm.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/cmd/status.py b/cloudinit/cmd/status.py
2index 3e5d0d0..d7aaee9 100644
3--- a/cloudinit/cmd/status.py
4+++ b/cloudinit/cmd/status.py
5@@ -93,6 +93,8 @@ def _is_cloudinit_disabled(disable_file, paths):
6 elif not os.path.exists(os.path.join(paths.run_dir, 'enabled')):
7 is_disabled = True
8 reason = 'Cloud-init disabled by cloud-init-generator'
9+ else:
10+ reason = 'Cloud-init enabled by systemd cloud-init-generator'
11 return (is_disabled, reason)
12
13
14@@ -127,10 +129,11 @@ def _get_status_details(paths):
15 status_detail = value
16 elif isinstance(value, dict):
17 errors.extend(value.get('errors', []))
18+ start = value.get('start') or 0
19 finished = value.get('finished') or 0
20- if finished == 0:
21+ if finished == 0 and start != 0:
22 status = STATUS_RUNNING
23- event_time = max(value.get('start', 0), finished)
24+ event_time = max(start, finished)
25 if event_time > latest_event:
26 latest_event = event_time
27 if errors:
28diff --git a/cloudinit/cmd/tests/test_status.py b/cloudinit/cmd/tests/test_status.py
29index 6d4a11e..a7c0a91 100644
30--- a/cloudinit/cmd/tests/test_status.py
31+++ b/cloudinit/cmd/tests/test_status.py
32@@ -93,6 +93,19 @@ class TestStatus(CiTestCase):
33 self.assertTrue(is_disabled, 'expected disabled cloud-init')
34 self.assertEqual('Cloud-init disabled by cloud-init-generator', reason)
35
36+ def test__is_cloudinit_disabled_false_when_enabled_in_systemd(self):
37+ '''Report enabled when systemd generator creates the enabled file.'''
38+ enabled_file = os.path.join(self.paths.run_dir, 'enabled')
39+ write_file(enabled_file, '')
40+ (is_disabled, reason) = wrap_and_call(
41+ 'cloudinit.cmd.status',
42+ {'uses_systemd': True,
43+ 'get_cmdline': 'something ignored'},
44+ status._is_cloudinit_disabled, self.disable_file, self.paths)
45+ self.assertFalse(is_disabled, 'expected enabled cloud-init')
46+ self.assertEqual(
47+ 'Cloud-init enabled by systemd cloud-init-generator', reason)
48+
49 def test_status_returns_not_run(self):
50 '''When status.json does not exist yet, return 'not run'.'''
51 self.assertFalse(
52@@ -137,8 +150,9 @@ class TestStatus(CiTestCase):
53 self.assertEqual(expected, m_stdout.getvalue())
54
55 def test_status_returns_running(self):
56- '''Report running when status file exists but isn't finished.'''
57- write_json(self.status_file, {'v1': {'init': {'finished': None}}})
58+ '''Report running when status exists with an unfinished stage.'''
59+ write_json(self.status_file,
60+ {'v1': {'init': {'start': 1, 'finished': None}}})
61 cmdargs = myargs(long=False, wait=False)
62 with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout:
63 retcode = wrap_and_call(
64@@ -338,7 +352,8 @@ class TestStatus(CiTestCase):
65
66 def test_status_main(self):
67 '''status.main can be run as a standalone script.'''
68- write_json(self.status_file, {'v1': {'init': {'finished': None}}})
69+ write_json(self.status_file,
70+ {'v1': {'init': {'start': 1, 'finished': None}}})
71 with self.assertRaises(SystemExit) as context_manager:
72 with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout:
73 wrap_and_call(
74diff --git a/cloudinit/config/cc_power_state_change.py b/cloudinit/config/cc_power_state_change.py
75index eba58b0..4da3a58 100644
76--- a/cloudinit/config/cc_power_state_change.py
77+++ b/cloudinit/config/cc_power_state_change.py
78@@ -194,6 +194,7 @@ def doexit(sysexit):
79
80
81 def execmd(exe_args, output=None, data_in=None):
82+ ret = 1
83 try:
84 proc = subprocess.Popen(exe_args, stdin=subprocess.PIPE,
85 stdout=output, stderr=subprocess.STDOUT)
86diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py
87index a9d21e7..530808c 100644
88--- a/cloudinit/config/cc_rh_subscription.py
89+++ b/cloudinit/config/cc_rh_subscription.py
90@@ -276,9 +276,8 @@ class SubscriptionManager(object):
91 cmd = ['attach', '--auto']
92 try:
93 return_out, return_err = self._sub_man_cli(cmd)
94- except util.ProcessExecutionError:
95- self.log_warn("Auto-attach failed with: "
96- "{0}]".format(return_err.strip()))
97+ except util.ProcessExecutionError as e:
98+ self.log_warn("Auto-attach failed with: {0}".format(e))
99 return False
100 for line in return_out.split("\n"):
101 if line is not "":
102diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
103index bad112f..aa468bc 100644
104--- a/cloudinit/distros/freebsd.py
105+++ b/cloudinit/distros/freebsd.py
106@@ -116,6 +116,7 @@ class Distro(distros.Distro):
107 (out, err) = util.subp(['ifconfig', '-a'])
108 ifconfigoutput = [x for x in (out.strip()).splitlines()
109 if len(x.split()) > 0]
110+ bsddev = 'NOT_FOUND'
111 for line in ifconfigoutput:
112 m = re.match('^\w+', line)
113 if m:
114@@ -347,15 +348,9 @@ class Distro(distros.Distro):
115 bymac[Distro.get_interface_mac(n)] = {
116 'name': n, 'up': self.is_up(n), 'downable': None}
117
118+ nics_with_addresses = set()
119 if check_downable:
120- nics_with_addresses = set()
121- ipv6 = self.get_ipv6()
122- ipv4 = self.get_ipv4()
123- for bytes_out in (ipv6, ipv4):
124- for i in ipv6:
125- nics_with_addresses.update(i)
126- for i in ipv4:
127- nics_with_addresses.update(i)
128+ nics_with_addresses = set(self.get_ipv4() + self.get_ipv6())
129
130 for d in bymac.values():
131 d['downable'] = (d['up'] is False or
132diff --git a/cloudinit/util.py b/cloudinit/util.py
133index 9976400..338fb97 100644
134--- a/cloudinit/util.py
135+++ b/cloudinit/util.py
136@@ -1587,6 +1587,10 @@ def mount_cb(device, callback, data=None, rw=False, mtype=None, sync=True):
137 mtypes = list(mtype)
138 elif mtype is None:
139 mtypes = None
140+ else:
141+ raise TypeError(
142+ 'Unsupported type provided for mtype parameter: {_type}'.format(
143+ _type=type(mtype)))
144
145 # clean up 'mtype' input a bit based on platform.
146 platsys = platform.system().lower()

Subscribers

People subscribed via source and target branches