You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by daan Hoogland <da...@gmail.com> on 2013/06/13 14:13:37 UTC
Review Request: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/
-----------------------------------------------------------
Review request for cloudstack.
Description
-------
double slash breaks windows based nfs servers [CLOUDSTACK-2968]
This addresses bug CLOUDSTACK-2968.
Diffs
-----
api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
server/src/com/cloud/storage/download/DownloadListener.java 1d48803
server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
Diff: https://reviews.apache.org/r/11861/diff/
Testing
-------
database analysis
Thanks,
daan Hoogland
Re: Review Request 11861: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
Posted by Amogh Vasekar <am...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/#review28410
-----------------------------------------------------------
Reminder-
Hi,
The review has been pending for long. Please update the request if this patch is still necessary or is to be discarded.
Thanks
- Amogh Vasekar
On Aug. 1, 2013, 11:39 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11861/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2013, 11:39 a.m.)
>
>
> Review request for cloudstack and John Burwell.
>
>
> Bugs: CLOUDSTACK-2968
> https://issues.apache.org/jira/browse/CLOUDSTACK-2968
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> double slash breaks windows based nfs servers [CLOUDSTACK-2968]
>
>
> Diffs
> -----
>
> api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
> core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
> core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
> core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
> engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
> engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
> server/src/com/cloud/storage/download/DownloadListener.java 1d48803
> server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
> server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
> utils/src/com/cloud/utils/FileUtil.java 74f4088
> utils/test/com/cloud/utils/FileUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11861/diff/
>
>
> Testing
> -------
>
> database analysis
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 11861: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
Posted by "Jenkins Cloudstack.org" <hu...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/#review24436
-----------------------------------------------------------
Review 11861 failed the build test : FAILURE
The url of build cloudstack-master-with-patch #101 is : http://jenkins.cloudstack.org/job/cloudstack-master-with-patch/101/
- Jenkins Cloudstack.org
On Aug. 1, 2013, 11:39 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11861/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2013, 11:39 a.m.)
>
>
> Review request for cloudstack and John Burwell.
>
>
> Bugs: CLOUDSTACK-2968
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> double slash breaks windows based nfs servers [CLOUDSTACK-2968]
>
>
> Diffs
> -----
>
> api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
> core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
> core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
> core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
> engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
> engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
> server/src/com/cloud/storage/download/DownloadListener.java 1d48803
> server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
> server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
> utils/src/com/cloud/utils/FileUtil.java 74f4088
> utils/test/com/cloud/utils/FileUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11861/diff/
>
>
> Testing
> -------
>
> database analysis
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 11861: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/
-----------------------------------------------------------
(Updated Aug. 1, 2013, 11:39 a.m.)
Review request for cloudstack and John Burwell.
Changes
-------
John, can you check if I need to rework or abandon this submission? thanks
Bugs: CLOUDSTACK-2968
Repository: cloudstack-git
Description
-------
double slash breaks windows based nfs servers [CLOUDSTACK-2968]
Diffs
-----
api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
server/src/com/cloud/storage/download/DownloadListener.java 1d48803
server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
utils/src/com/cloud/utils/FileUtil.java 74f4088
utils/test/com/cloud/utils/FileUtilTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/11861/diff/
Testing
-------
database analysis
Thanks,
daan Hoogland
Re: Review Request: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/
-----------------------------------------------------------
(Updated June 15, 2013, 12:41 p.m.)
Review request for cloudstack.
Changes
-------
extracted fixPath functionality
addressed logging comments
Description
-------
double slash breaks windows based nfs servers [CLOUDSTACK-2968]
This addresses bug CLOUDSTACK-2968.
Diffs (updated)
-----
api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
server/src/com/cloud/storage/download/DownloadListener.java 1d48803
server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
utils/src/com/cloud/utils/FileUtil.java 74f4088
utils/test/com/cloud/utils/FileUtilTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/11861/diff/
Testing
-------
database analysis
Thanks,
daan Hoogland
Re: Review Request: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
Posted by daan Hoogland <da...@gmail.com>.
> On June 14, 2013, 3:24 p.m., John Burwell wrote:
> > api/src/com/cloud/storage/template/TemplateInfo.java, line 32
> > <https://reviews.apache.org/r/11861/diff/2/?file=305010#file305010line32>
> >
> > Null is not a sufficient check for blank. Use StringUtils#isBlank to catch both null and instances containing only spaces.
empty or spaces are not harmfull in this location
> On June 14, 2013, 3:24 p.m., John Burwell wrote:
> > core/src/com/cloud/storage/template/TemplateLocation.java, line 168
> > <https://reviews.apache.org/r/11861/diff/2/?file=305013#file305013line168>
> >
> > Why are is the path being manipulated in addition to the work being down by the TemplateInfo class? These rules should be completely encapsulated in the TemplateInfo class.
The two manipulations are not exactly the same. File.separator is not always "/".
> On June 14, 2013, 3:24 p.m., John Burwell wrote:
> > api/src/com/cloud/storage/template/TemplateInfo.java, line 38
> > <https://reviews.apache.org/r/11861/diff/2/?file=305010#file305010line38>
> >
> > WARN should be used to let an operator know there is a condition in the system that could lead to instability. A data cleanup will not result in eventual system instability. I recommend converting to debug or trace.
The point is that a mixed posix-paths/UNC system triggered this fix. A double slash has double meaning in such an environment. However the error, be it human or system generated, may or may not destabalize cloudstack, so I will stick with the info. It is certainly not debug in my opinion. It is not a bug that needs debugging.
quoting Hiroaki KAWAI:
If something went wrong and double slash was passed to
Winfows based NFS, the reason may A) there was another
code that generates double slash B) cloudstack configuration
or something user input was bad C) some path components became
empty string because of database error or something unexpeceted
D) cloudstack is really being attacked etc.
- daan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/#review21907
-----------------------------------------------------------
On June 15, 2013, 12:41 p.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11861/
> -----------------------------------------------------------
>
> (Updated June 15, 2013, 12:41 p.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> double slash breaks windows based nfs servers [CLOUDSTACK-2968]
>
>
> This addresses bug CLOUDSTACK-2968.
>
>
> Diffs
> -----
>
> api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
> core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
> core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
> core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
> engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
> engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
> server/src/com/cloud/storage/download/DownloadListener.java 1d48803
> server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
> server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
> utils/src/com/cloud/utils/FileUtil.java 74f4088
> utils/test/com/cloud/utils/FileUtilTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/11861/diff/
>
>
> Testing
> -------
>
> database analysis
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
Posted by John Burwell <jb...@basho.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/#review21907
-----------------------------------------------------------
api/src/com/cloud/storage/template/TemplateInfo.java
<https://reviews.apache.org/r/11861/#comment45197>
.getName() is unnecessary. Just pass the class:
private static Logger s_logger = Logger.getLogger(TemplateInfo.class);
api/src/com/cloud/storage/template/TemplateInfo.java
<https://reviews.apache.org/r/11861/#comment45198>
Null is not a sufficient check for blank. Use StringUtils#isBlank to catch both null and instances containing only spaces.
api/src/com/cloud/storage/template/TemplateInfo.java
<https://reviews.apache.org/r/11861/#comment45199>
WARN should be used to let an operator know there is a condition in the system that could lead to instability. A data cleanup will not result in eventual system instability. I recommend converting to debug or trace.
api/src/com/cloud/storage/template/TemplateInfo.java
<https://reviews.apache.org/r/11861/#comment45200>
Why are fixing the path in both the getter and setter? I recommend fixing it in either method, not both.
core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java
<https://reviews.apache.org/r/11861/#comment45201>
See previous comment regarding logger initialization.
core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java
<https://reviews.apache.org/r/11861/#comment45202>
Extract this method to com.cloud.utils.FileUtil and reference from both CreateEntityDownloadURLCommand and TemplateInfo. Also, add unit tests for it.
core/src/com/cloud/storage/template/TemplateLocation.java
<https://reviews.apache.org/r/11861/#comment45203>
Why are is the path being manipulated in addition to the work being down by the TemplateInfo class? These rules should be completely encapsulated in the TemplateInfo class.
engine/schema/src/com/cloud/storage/VMTemplateHostVO.java
<https://reviews.apache.org/r/11861/#comment45204>
See previous note about logger initialization
engine/schema/src/com/cloud/storage/VMTemplateHostVO.java
<https://reviews.apache.org/r/11861/#comment45205>
See previous note about extraction of this method and unit testing.
server/src/com/cloud/storage/download/DownloadMonitorImpl.java
<https://reviews.apache.org/r/11861/#comment45207>
Wrap in an if (s_logger.isDebugEnabled()) block to avoid expensive string concatenation when DEBUG logging is not enabled.
server/src/com/cloud/storage/download/DownloadMonitorImpl.java
<https://reviews.apache.org/r/11861/#comment45206>
Wrap in an if (s_logger.isDebugEnabled()) block to avoid expensive string concatenation when DEBUG logging is not enabled.
server/src/com/cloud/template/TemplateManagerImpl.java
<https://reviews.apache.org/r/11861/#comment45208>
Wrap in an if (s_logger.isDebugEnabled()) block to avoid expensive string concatenation when DEBUG logging is not enabled.
- John Burwell
On June 14, 2013, 2:18 p.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11861/
> -----------------------------------------------------------
>
> (Updated June 14, 2013, 2:18 p.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> double slash breaks windows based nfs servers [CLOUDSTACK-2968]
>
>
> This addresses bug CLOUDSTACK-2968.
>
>
> Diffs
> -----
>
> api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
> core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
> core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
> core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
> engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
> engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
> server/src/com/cloud/storage/download/DownloadListener.java 1d48803
> server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
> server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
> server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
>
> Diff: https://reviews.apache.org/r/11861/diff/
>
>
> Testing
> -------
>
> database analysis
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request: double slash fix for windows based nfs servers
[CLOUDSTACK-2968]
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11861/
-----------------------------------------------------------
(Updated June 14, 2013, 2:18 p.m.)
Review request for cloudstack.
Changes
-------
made conditional warning message on autofix paths
Description
-------
double slash breaks windows based nfs servers [CLOUDSTACK-2968]
This addresses bug CLOUDSTACK-2968.
Diffs (updated)
-----
api/src/com/cloud/storage/template/TemplateInfo.java 6559d73
core/src/com/cloud/agent/api/storage/CreateEntityDownloadURLCommand.java 98a957f
core/src/com/cloud/agent/api/storage/DownloadAnswer.java bb7b8a9
core/src/com/cloud/storage/template/TemplateLocation.java 58d023a
engine/schema/src/com/cloud/storage/VMTemplateHostVO.java b8dfc41
engine/storage/src/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java a6880c3
server/src/com/cloud/storage/download/DownloadListener.java 1d48803
server/src/com/cloud/storage/download/DownloadMonitorImpl.java f72a563
server/src/com/cloud/template/HypervisorTemplateAdapter.java 322f32e
server/src/com/cloud/template/TemplateManagerImpl.java 517d4ba
Diff: https://reviews.apache.org/r/11861/diff/
Testing
-------
database analysis
Thanks,
daan Hoogland