[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[rsafonseca]

GitHub user rsafonseca opened a pull request:

https://github.com/apache/cloudstack/pull/460

Fix findbugs encoding issue in VmwareStorageProcessor.java

Any encoding would do fine as it's just used to generate a UUID. Sticking 
with UTF-8 for consistency

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/rsafonseca/cloudstack findbugs75

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/460.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #460


commit 8cb5f19dcc1f6e0775ddfb4802f30e6463499958
Author: Rafael da Fonseca 
Date:   2015-06-15T17:15:55Z

Fix findbugs encoding issue in VmwareStorageProcessor.java
Any encoding would do fine as it's just used to generate a UUID. Sticking 
with UTF-8 for consistency




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/460#discussion_r32465788
  
--- Diff: 
plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java
 ---
@@ -2265,7 +2265,7 @@ public Answer forgetObject(ForgetObjectCmd cmd) {
 }
 
 private static String deriveTemplateUuidOnHost(VmwareHypervisorHost 
hyperHost, String storeIdentifier, String templateName) {
-String templateUuid = UUID.nameUUIDFromBytes((templateName + "@" + 
storeIdentifier + "-" + hyperHost.getMor().getValue()).getBytes()).toString();
+String templateUuid = UUID.nameUUIDFromBytes((templateName + "@" + 
storeIdentifier + "-" + 
hyperHost.getMor().getValue()).getBytes("UTF-8")).toString();
--- End diff --

In other  cases I used Charset.defaultCharset(). Is there a reason to 
explicitly use "UTF8"?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[rsafonseca]

Github user rsafonseca commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/460#discussion_r32469324
  
--- Diff: 
plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageProcessor.java
 ---
@@ -2265,7 +2265,7 @@ public Answer forgetObject(ForgetObjectCmd cmd) {
 }
 
 private static String deriveTemplateUuidOnHost(VmwareHypervisorHost 
hyperHost, String storeIdentifier, String templateName) {
-String templateUuid = UUID.nameUUIDFromBytes((templateName + "@" + 
storeIdentifier + "-" + hyperHost.getMor().getValue()).getBytes()).toString();
+String templateUuid = UUID.nameUUIDFromBytes((templateName + "@" + 
storeIdentifier + "-" + 
hyperHost.getMor().getValue()).getBytes("UTF-8")).toString();
--- End diff --

Unless you're getting platform specifc data, i think it's best to always 
stick to use the same charset, to avoid different values from being generated 
in different platforms whenever possible. Ideally, we should convert all data 
that comes into the application to a standard charset that supports 
internationalization such as UTF-8, and just use default platform charset where 
applicable, when consuming data from platform specific sources.
Using Charset.defaultCharset() will require an extra import and increase 
the overall size of the class, altough instantiating a String also has its own 
overhead, i believe it to be less so.. we can also remove string instantiation 
at runtime by defining a constant, which will also increase runtime efficiency.
Of course this is only my point of view, please comment if you feel 
otherwise :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[rsafonseca]

Github user rsafonseca commented on the pull request:

https://github.com/apache/cloudstack/pull/460#issuecomment-112232241
  
Here's an example of an instance where i also removed a 
Charset.defaultCharset() that was placed in the reading of a socket, where 
cloudstack controls both ends -> 
https://github.com/apache/cloudstack/pull/400/files
If ConsoleProxyCmdHandler.java would run on a different encoding, you might 
have issues, so i changes all classes that used this to use UTF-8 explicitly. 
Charset.defaultCharset() should still work on 
CitrixConsoleProxyLoadCommandWrapper.java (if this runs on the agent side, 
didn't check) since platform that Xenserver  runs on also uses UTF-8 as a 
default.. but in the off chance the sysadmin screwed with the java defaults or 
this isn't running on the agent side, it might then fail.. this ensures it 
would work properly no matter what, it's always safer to do this if you control 
both the reader and the writer.

And here's an example where i think it's most suited to use 
Charset.defaultCharset() , the output of a shell command:
https://github.com/apache/cloudstack/pull/396

It's also possible to screw with the output of this from the system's side 
(although very unlikely), but this is the best approximation of 100% fail-proof 
of getting proper encoding of the input data.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/460#issuecomment-112313538
  
thanks, LGTM


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/460


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/460#issuecomment-112333921
  
I shouldn't have:( exception uncought. Really curious why the pull request 
builder accepted this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[rsafonseca]

Github user rsafonseca commented on the pull request:

https://github.com/apache/cloudstack/pull/460#issuecomment-112338005
  
my bad there.. will surround in try/catch block and update


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/460#issuecomment-112339045
  
please make it a seperate fix, not an update. I jumped the gun.

On Tue, Jun 16, 2015 at 10:36 AM, Rafael da Fonseca <
notificati...@github.com> wrote:

> my bad there.. will surround in try/catch block and update
>
> —
> Reply to this email directly or view it on GitHub
> .
>



-- 
Daan



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[rsafonseca]

Github user rsafonseca commented on the pull request:

https://github.com/apache/cloudstack/pull/460#issuecomment-112344047
  
@DaanHoogland done!
https://github.com/apache/cloudstack/pull/464
I should have checked travis for this one, still my bad :(


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Re: [GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...

[Daan Hoogland]

I think it's caught and also that the pull request builder doesn't
build -Dnoredist. I will check and fix if so

On Tue, Jun 16, 2015 at 10:36 AM, rsafonseca  wrote:
> Github user rsafonseca commented on the pull request:
>
> https://github.com/apache/cloudstack/pull/460#issuecomment-112338005
>
> my bad there.. will surround in try/catch block and update
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
> with INFRA.
> ---



-- 
Daan