You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by rsafonseca <gi...@git.apache.org> on 2015/06/15 19:16:29 UTC
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
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 <rs...@gmail.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by DaanHoogland <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by DaanHoogland <gi...@git.apache.org>.
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 <
notifications@github.com> wrote:
> my bad there.. will surround in try/catch block and update
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cloudstack/pull/460#issuecomment-112338005>.
>
--
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by rsafonseca <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by DaanHoogland <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by rsafonseca <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by Daan Hoogland <da...@gmail.com>.
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 <gi...@git.apache.org> 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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
--
Daan
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by rsafonseca <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by DaanHoogland <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Fix findbugs encoding issue in VmwareStor...
Posted by rsafonseca <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---