[CLOUDSTACK-10156] Fix Coverity new problems CID(1349987, 1349986, 1347248) by rafaelweingartner · Pull Request #2332 · apache/cloudstack

rafaelweingartner · 2017-11-17T16:43:38Z

No description provided.

yadvr · 2017-11-17T18:02:08Z


    @Override
    public long getEntityOwnerId() {
-        Long accountId = _accountService.getActiveAccountByName(accountName, domainId).getAccountId();


This can be kept to know who made the API call

Ah, so that is what this is for.
I will create a log then.

yadvr · 2017-11-17T18:04:55Z

LGTM

yadvr · 2017-11-22T07:08:20Z

-        if (accountId == null) {
-            return CallContext.current().getCallingAccount().getId();
-        }
+        s_logger.debug(String.format("Account [id=%d, name=%s, domain=%d] is executing method: %s ", accountId, accountName, domainId, getClass().getSimpleName()));


Logging is not necessary here. The user/account making the API call is already logged, along with API name+args.

yadvr · 2017-11-22T07:08:56Z

@blueorangutan package

blueorangutan · 2017-11-22T07:09:19Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-11-22T07:41:09Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1280

yadvr · 2017-11-22T09:17:48Z

@blueorangutan test

blueorangutan · 2017-11-22T09:18:19Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2017-11-22T21:00:35Z

Trillian test result (tid-1680)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28933 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2332-t1680-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_host_annotations.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermitten failure detected: /marvin/tests/smoke/test_ssvm.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Test completed. 60 look OK, 3 have error(s)

Test	Result	Time (s)	Test File
test_01_vpc_remote_access_vpn	`Failure`	45.82	test_vpc_vpn.py
test_04_rvpc_privategw_static_routes	`Failure`	243.57	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	97.88	test_privategw_acl.py
test_02_vpc_privategw_static_routes	`Failure`	183.14	test_privategw_acl.py
test_01_vpc_privategw_acl	`Failure`	56.85	test_privategw_acl.py
test_05_add_annotation_for_invalid_entityType	`Error`	0.06	test_host_annotations.py
test_change_service_offering_for_vm_with_snapshots	Skipped	0.00	test_vm_snapshots.py
test_09_copy_delete_template	Skipped	0.02	test_templates.py
test_06_copy_template	Skipped	0.00	test_templates.py
test_static_role_account_acls	Skipped	0.03	test_staticroles.py
test_11_ss_nfs_version_on_ssvm	Skipped	0.02	test_ssvm.py
test_01_scale_vm	Skipped	0.00	test_scale_vm.py
test_01_primary_storage_iscsi	Skipped	0.08	test_primary_storage.py
test_vm_nic_adapter_vmxnet3	Skipped	0.00	test_nic_adapter_type.py
test_03_nic_multiple_vmware	Skipped	1.07	test_nic.py
test_nested_virtualization_vmware	Skipped	0.00	test_nested_virtualization.py
test_06_copy_iso	Skipped	0.00	test_iso.py
test_list_ha_for_host_valid	Skipped	0.02	test_hostha_simulator.py
test_list_ha_for_host_invalid	Skipped	0.02	test_hostha_simulator.py
test_list_ha_for_host	Skipped	0.02	test_hostha_simulator.py
test_hostha_enable_feature_without_setting_provider	Skipped	0.03	test_hostha_simulator.py
test_hostha_enable_feature_valid	Skipped	0.04	test_hostha_simulator.py
test_hostha_disable_feature_valid	Skipped	0.02	test_hostha_simulator.py
test_hostha_configure_invalid_provider	Skipped	0.02	test_hostha_simulator.py
test_hostha_configure_default_driver	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_recovering	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_fenced	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_degraded	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_available	Skipped	0.02	test_hostha_simulator.py
test_ha_multiple_mgmt_server_ownership	Skipped	0.02	test_hostha_simulator.py
test_ha_list_providers	Skipped	0.03	test_hostha_simulator.py
test_ha_enable_feature_invalid	Skipped	0.02	test_hostha_simulator.py
test_ha_disable_feature_invalid	Skipped	0.02	test_hostha_simulator.py
test_ha_configure_enabledisable_across_clusterzones	Skipped	0.03	test_hostha_simulator.py
test_configure_ha_provider_valid	Skipped	0.02	test_hostha_simulator.py
test_configure_ha_provider_invalid	Skipped	0.03	test_hostha_simulator.py
test_deploy_vgpu_enabled_vm	Skipped	0.03	test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support	Skipped	0.04	test_deploy_vgpu_enabled_vm.py

yadvr · 2017-11-22T21:08:12Z

Tests lgtm, fixed redundant logging. @rafaelweingartner can you check and add a jira ID, then we can merge it. Thanks.

rafaelweingartner · 2017-11-22T21:33:11Z

@@ -120,8 +119,8 @@ public String getCommandName() {
    @Override
    public long getEntityOwnerId() {
        Long accountId = _accountService.getActiveAccountByName(accountName, domainId).getAccountId();


@rhtyd this will never be null. The method getAccountId returns a primitive long. Therefore, this check that you added is not needed. That is why when you asked to preserve the call _accountService.getActiveAccountByName(accountName, domainId).getAccountId(); I wrote a code that was logging the value. Otherwise it would be a call that did not make sense.

If you take a look at the first commit and compare it with HEAD, you are going to see the following code in HEAD:

if (accountId == null) {
return CallContext.current().getCallingAccount().getId();
}
return Account.ACCOUNT_ID_SYSTEM;

accountId is never null, that is why I removed that block, and left only the return Account.ACCOUNT_ID_SYSTEM; code. If you think that we have to return the value of accountService.getActiveAccountByName(accountName, domainId).getAccountId(); we can simply return it as it is a primitive long and will never be null

Sure, let's return long value of accountId.

Great. I will change this then

rafaelweingartner · 2017-11-22T21:35:07Z

@rhtyd sure I can create a Jira ticket. I have some remarks in the code you are introducing.

I will create the Jira while we check that point there.

rafaelweingartner · 2017-11-22T21:57:44Z

@rhtyd done!

yadvr · 2017-11-23T06:10:28Z

@rafaelweingartner I've fixed a minor spacing/styling issue. LGTM merging.

rafaelweingartner · 2017-11-23T10:19:39Z

Thanks @rhtyd !
I am using ACS eclipse code formatter, I will check next time if it has some misconfiguration, or if I disabled something in the save actions.

yadvr reviewed Nov 17, 2017

View reviewed changes

yadvr reviewed Nov 22, 2017

View reviewed changes

rafaelweingartner commented Nov 22, 2017

View reviewed changes

rafaelweingartner force-pushed the fixCoverityCid-1349987-1349986-1347248 branch from 6a2ebfb to cb60ee5 Compare November 22, 2017 21:44

Fix Coverity new problems CID(1349987, 1349986, 1347248)

2c6fdf0

rafaelweingartner force-pushed the fixCoverityCid-1349987-1349986-1347248 branch from cb60ee5 to 2c6fdf0 Compare November 22, 2017 21:48

rafaelweingartner changed the title ~~Fix Coverity new problems CID(1349987, 1349986, 1347248)~~ [CLOUDSTACK-10156] Fix Coverity new problems CID(1349987, 1349986, 1347248) Nov 22, 2017

Fix spacing

6eb922e

yadvr merged commit cb9c7ad into apache:master Nov 23, 2017

Conversation

rafaelweingartner commented Nov 17, 2017

Uh oh!

yadvr Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

yadvr commented Nov 17, 2017

Uh oh!

yadvr Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Nov 22, 2017

Uh oh!

blueorangutan commented Nov 22, 2017

Uh oh!

blueorangutan commented Nov 22, 2017

Uh oh!

yadvr commented Nov 22, 2017

Uh oh!

blueorangutan commented Nov 22, 2017

Uh oh!

blueorangutan commented Nov 22, 2017

Uh oh!

yadvr commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelweingartner Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

yadvr Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Nov 22, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Nov 22, 2017

Uh oh!

rafaelweingartner commented Nov 22, 2017

Uh oh!

yadvr commented Nov 23, 2017

Uh oh!

rafaelweingartner commented Nov 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yadvr Nov 22, 2017 •

edited

Loading

yadvr commented Nov 22, 2017 •

edited

Loading