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 <DH...@schubergphilis.com> on 2013/06/13 14:15:25 UTC
committer wanted for review
H,
Can someone look at Review Request #11861<https://reviews.apache.org/r/11861/> for me please?
Thanks,
Daan Hoogland
Re: committer wanted for review
Posted by Daan Hoogland <da...@gmail.com>.
On 2013/6/18 16:49 , John Burwell wrote:
> Second, please don't take my feedback as passing judgements such as things being ugly or
don't worry, I like the discussion and i don't mind losing an argument
if the best solution arises from it. Let's see about that.
--
<http://daan.sbpad6.nl/>
Re: committer wanted for review
Posted by John Burwell <jb...@basho.com>.
On Jun 18, 2013, at 1:09 AM, Daan Hoogland <da...@gmail.com> wrote:
> John,
>
> I like your ground principles, I will keep looking a bit for a better spot
> to solve the problem.
>
> I am not sure your arguments about technical debt and user input validation
> apply in the situation at hand.
>
> A user will probably be just annoyed if we reject a valid posix path
> because it does not comply to unc or vice versa. So the policy of being
> strict on the output lenient on the input is maybe desirable.
As previous explained by Hiroaki, we have four issues around NFS path specifications. The patch, as currently constituted address one of the issues while masking another. Therefore, we are going to have to address this issue again in the future to address the other issues, and, likely, deal with the ramifications of having CloudStack incorrectly generating paths. While ugly, the current code breaks -- giving us some trail to to the root cause of the problem. With this fix, we are masking the issue of CloudStack code generating invalid paths. Based on this understanding, I feel that we are expanding the technical debt around paths, not reducing it.
If I am understanding the UI flow correctly, we are talking about the path entered when creating secondary storage. If so, the end user is a system administrator/operator who shouldn't find such a requirement onerous. For input validation, we would need to add some on-line help pointing folks to the relevant RFCs explaining the format.
>
> A more pragmatic argument, which i make maybe without being hindered by
> deep knowledge of cloudstack code, is that a path may be constructed in a
> distributed way and the error may occur at several locations. The path my
> users found had two instances of double slashes. It is both cheap,
> maintainable and robust to solve it in one location. I do not pretend that
> I have found that location however.
The situation you describe seems to beg for a dedicated Path immutable value object with either static factory methods or an associated builder than understands the rules around concatenation. The more I think about the invocation of the fixPath method is that are typically lack the context to understand intent. Providing a value object and explicit creational mechanisms allows clients to express intent and for the Storage layer to establish a deterministic contact for clients.
>
> Then their is the question of whether this is an improvement or really only
> an ugly workaround. If you say yes to the latter, my submission should be
> refused altogether.
>
> Having made those arguments; The priority on has been lowered by my users
> so much more time to work on it is not available. It was fun so far and i
> will have another dive at the code to find a good spot to solve this
> particular problem.
First and foremost, I appreciate anyone who wants to roll up their sleeves and help improve CloudStack (particularly storage). Second, please don't take my feedback as passing judgements such as things being ugly or pretty. Instead, the review process has evolved our understanding of the problem to be larger than just Windows clients. From my perspective, this work has expanded our understanding of the system and some lurking issues. I do think we need to re-approach the fix in light of our deeper understanding. That being said, I think a new approach would actually be less code ….
>
> thanks for your comments John,
> Daan
>
>
> On Mon, Jun 17, 2013 at 6:49 PM, John Burwell <jb...@basho.com> wrote:
>
>> Daan,
>>
>> Please see my comments in-line below.
>>
>> Thanks,
>> -John
>>
>> On Jun 17, 2013, at 9:40 AM, Daan Hoogland <da...@gmail.com>
>> wrote:
>>
>>> John,
>>>
>>> If I understand it correctly, you are stating that my take on the
>> solution
>>> is 'not done/not the way to go'?
>>
>>>
>>> For the record the case I solved was an instance of A, but I would not
>> call
>>> it adding technical debt. A arose from existing code in combination of a
>>> requirement to work with a non-posix-path compliant (but unc) nfs server.
>>
>> From my perspective, it is technical debt because the solution, as
>> implemented, is masking/compensating for underlying defects. I think we
>> should fix the underlying defects, input validation and value persistence,
>> rather than trying to compensate for it in the storage layer. We also
>> likely need some type of utility/functionality to upgrade tools to identify
>> invalid path data in existing installations for correction.
>>
>>>
>>> regards,
>>>
>>>
>>> On Mon, Jun 17, 2013 at 2:01 PM, John Burwell <jb...@basho.com>
>> wrote:
>>>
>>>> All,
>>>>
>>>> Please see my comments in-line below.
>>>>
>>>> Thanks,
>>>> -John
>>>>
>>>> On Jun 15, 2013, at 6:11 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp>
>>>> wrote:
>>>>
>>>>> Probably we've agreed on that double slash should not
>>>>> generated by cloudstack.
>>>>>
>>>>> 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.,
>>>>
>>>> A indicates that we adding technical debt and later defects to the
>> system.
>>>> We need to fix upstream for correctness before it rots further. B sound
>>>> like a case for stronger input validation rather than a "fix up" on the
>>>> backend. C seems like we need to be more careful in how we persist and
>>>> retrieve the information from the database. The more we discuss this
>>>> solution, the more this feels like a front-end input validation and
>>>> database persistence issue. Treating it this way would obviate any
>>>> security issues or logging needs.
>>>>
>>>>>
>>>>> Anyway, double slash should not happen and the admins should be
>>>>> able to know when the NFS layer got that sequence.
>>>>> I'd prefer WARN for this reason, but INFO may do as well.
>>>>> I don't have strong opinion on log level.
>>>>
>>>>
>>>> If it shouldn't happen then we should be rejecting the data as part of
>>>> input validation and no allowing it to be persisted.
>>>>
>>>>>
>>>>> In addition to that, "auto-fix" may not be a "fix" for example in
>>>>> case "C". I don't want to see autofix code in many places,
>>>>> "auto-fix" might be a "fix" where the path is really passed to
>>>>> NFS layer.
>>>>>
>>>>> Another approach to double-slash is just reject the input and raise
>>>>> a CloudstackRuntimeException.
>>>>> But I'd prefer auto-fix because of case "A" at this moment…
>>>>
>>>> Originally, I thought this fix was the equivalent of escaping a URL or
>>>> HTML string. Now that I understand it more fully, I believe we need to
>>>> throw a CloudRuntimeException to ferret out code generating incorrectly
>>>> formatted input.
>>>>
>>>>>
>>>>>
>>>>> (2013/06/15 18:01), Daan Hoogland wrote:
>>>>>> H John,
>>>>>>
>>>>>> Yes, actually I was going to make it info level but you swapped me of
>> my
>>>>>> feet with your remark.
>>>>>>
>>>>>> 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, does not destabalize cloudstack in
>> any
>>>>>> way, so I will stick with the info. It is certainly not debug in my
>>>>>> opinion. It is not a bug that needs debugging.
>>>>>>
>>>>>> Of course a deeper understanding of cloudstack might change my
>> position
>>>> on
>>>>>> the issue.
>>>>>>
>>>>>> regards,
>>>>>> Daan
>>>>>>
>>>>>>
>>>>>> On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jb...@basho.com>
>>>> wrote:
>>>>>>
>>>>>>> Daan,
>>>>>>>
>>>>>>> Since a WARN indicates a condition that could lead to system
>>>> instability,
>>>>>>> many folks configure their log analysis to trigger notifications on
>>>> WARN
>>>>>>> and INFO. Does escaping a character in a path warrant meet that
>>>> criteria?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> -John
>>>>>>>
>>>>>>> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <daan.hoogland@gmail.com
>>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> H John,
>>>>>>>>
>>>>>>>> I browsed through your comments and most I will apply. There is one
>>>> where
>>>>>>>> you contradict Hiroaki. This is about the logging level for
>> reporting
>>>> a
>>>>>>>> changed path. I am going to follow my heart at this unless there is
>> a
>>>>>>>> project directive on it.
>>>>>>>>
>>>>>>>> regards,
>>>>>>>> Daan
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Daan,
>>>>>>>>>
>>>>>>>>> I just looked through the review request, and published my
>> comments.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> -John
>>>>>>>>>
>>>>>>>>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <
>> daan.hoogland@gmail.com
>>>>>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hiroaki,
>>>>>>>>>>
>>>>>>>>>> - auto-fix may happen where it is really required
>>>>>>>>>>>
>>>>>>>>>> I do not have a clear view on this, so I took the approach of
>> better
>>>>>>> safe
>>>>>>>>>> then sorry. The submitted is what works. I don't see how the
>>>> auto-fix
>>>>>>>>>> should ever be needed if the source is fixed. Hope you can live
>> with
>>>>>>>>> this.
>>>>>>>>>>
>>>>>>>>>>> - and if auto-fix happens, it should log it with
>>>>>>>>>>> WARN level.
>>>>>>>>>>
>>>>>>>>>> Applied
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> regards,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <
>>>>>>> daan.hoogland@gmail.com
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Thanks Hiroaki,
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
>>>>>>>>> kawai@stratosphere.co.jp>wrote:
>>>>>>>>>>>
>>>>>>>>>>>> I'd suggest:
>>>>>>>>>>>> - fix the generation of double slash itself
>>>>>>>>>>>>
>>>>>>>>>>> Is in the patch
>>>>>>>>>>>
>>>>>>>>>>>> - auto-fix may happen where it is really required
>>>>>>>>>>>> - and if auto-fix happens, it should log it with
>>>>>>>>>>>> WARN level.
>>>>>>>>>>>
>>>>>>>>>>> Good point, I will up the level in an update.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> (2013/06/13 21:15), Daan Hoogland wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> H,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Can someone look at Review Request #11861<
>>>> https://reviews.apache.**
>>>>>>>>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me
>>>> please?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Daan Hoogland
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>
>>
Re: committer wanted for review
Posted by Daan Hoogland <da...@gmail.com>.
John,
I like your ground principles, I will keep looking a bit for a better spot
to solve the problem.
I am not sure your arguments about technical debt and user input validation
apply in the situation at hand.
A user will probably be just annoyed if we reject a valid posix path
because it does not comply to unc or vice versa. So the policy of being
strict on the output lenient on the input is maybe desirable.
A more pragmatic argument, which i make maybe without being hindered by
deep knowledge of cloudstack code, is that a path may be constructed in a
distributed way and the error may occur at several locations. The path my
users found had two instances of double slashes. It is both cheap,
maintainable and robust to solve it in one location. I do not pretend that
I have found that location however.
Then their is the question of whether this is an improvement or really only
an ugly workaround. If you say yes to the latter, my submission should be
refused altogether.
Having made those arguments; The priority on has been lowered by my users
so much more time to work on it is not available. It was fun so far and i
will have another dive at the code to find a good spot to solve this
particular problem.
thanks for your comments John,
Daan
On Mon, Jun 17, 2013 at 6:49 PM, John Burwell <jb...@basho.com> wrote:
> Daan,
>
> Please see my comments in-line below.
>
> Thanks,
> -John
>
> On Jun 17, 2013, at 9:40 AM, Daan Hoogland <da...@gmail.com>
> wrote:
>
> > John,
> >
> > If I understand it correctly, you are stating that my take on the
> solution
> > is 'not done/not the way to go'?
>
> >
> > For the record the case I solved was an instance of A, but I would not
> call
> > it adding technical debt. A arose from existing code in combination of a
> > requirement to work with a non-posix-path compliant (but unc) nfs server.
>
> From my perspective, it is technical debt because the solution, as
> implemented, is masking/compensating for underlying defects. I think we
> should fix the underlying defects, input validation and value persistence,
> rather than trying to compensate for it in the storage layer. We also
> likely need some type of utility/functionality to upgrade tools to identify
> invalid path data in existing installations for correction.
>
> >
> > regards,
> >
> >
> > On Mon, Jun 17, 2013 at 2:01 PM, John Burwell <jb...@basho.com>
> wrote:
> >
> >> All,
> >>
> >> Please see my comments in-line below.
> >>
> >> Thanks,
> >> -John
> >>
> >> On Jun 15, 2013, at 6:11 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp>
> >> wrote:
> >>
> >>> Probably we've agreed on that double slash should not
> >>> generated by cloudstack.
> >>>
> >>> 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.,
> >>
> >> A indicates that we adding technical debt and later defects to the
> system.
> >> We need to fix upstream for correctness before it rots further. B sound
> >> like a case for stronger input validation rather than a "fix up" on the
> >> backend. C seems like we need to be more careful in how we persist and
> >> retrieve the information from the database. The more we discuss this
> >> solution, the more this feels like a front-end input validation and
> >> database persistence issue. Treating it this way would obviate any
> >> security issues or logging needs.
> >>
> >>>
> >>> Anyway, double slash should not happen and the admins should be
> >>> able to know when the NFS layer got that sequence.
> >>> I'd prefer WARN for this reason, but INFO may do as well.
> >>> I don't have strong opinion on log level.
> >>
> >>
> >> If it shouldn't happen then we should be rejecting the data as part of
> >> input validation and no allowing it to be persisted.
> >>
> >>>
> >>> In addition to that, "auto-fix" may not be a "fix" for example in
> >>> case "C". I don't want to see autofix code in many places,
> >>> "auto-fix" might be a "fix" where the path is really passed to
> >>> NFS layer.
> >>>
> >>> Another approach to double-slash is just reject the input and raise
> >>> a CloudstackRuntimeException.
> >>> But I'd prefer auto-fix because of case "A" at this moment…
> >>
> >> Originally, I thought this fix was the equivalent of escaping a URL or
> >> HTML string. Now that I understand it more fully, I believe we need to
> >> throw a CloudRuntimeException to ferret out code generating incorrectly
> >> formatted input.
> >>
> >>>
> >>>
> >>> (2013/06/15 18:01), Daan Hoogland wrote:
> >>>> H John,
> >>>>
> >>>> Yes, actually I was going to make it info level but you swapped me of
> my
> >>>> feet with your remark.
> >>>>
> >>>> 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, does not destabalize cloudstack in
> any
> >>>> way, so I will stick with the info. It is certainly not debug in my
> >>>> opinion. It is not a bug that needs debugging.
> >>>>
> >>>> Of course a deeper understanding of cloudstack might change my
> position
> >> on
> >>>> the issue.
> >>>>
> >>>> regards,
> >>>> Daan
> >>>>
> >>>>
> >>>> On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jb...@basho.com>
> >> wrote:
> >>>>
> >>>>> Daan,
> >>>>>
> >>>>> Since a WARN indicates a condition that could lead to system
> >> instability,
> >>>>> many folks configure their log analysis to trigger notifications on
> >> WARN
> >>>>> and INFO. Does escaping a character in a path warrant meet that
> >> criteria?
> >>>>>
> >>>>> Thanks,
> >>>>> -John
> >>>>>
> >>>>> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <daan.hoogland@gmail.com
> >
> >>>>> wrote:
> >>>>>
> >>>>>> H John,
> >>>>>>
> >>>>>> I browsed through your comments and most I will apply. There is one
> >> where
> >>>>>> you contradict Hiroaki. This is about the logging level for
> reporting
> >> a
> >>>>>> changed path. I am going to follow my heart at this unless there is
> a
> >>>>>> project directive on it.
> >>>>>>
> >>>>>> regards,
> >>>>>> Daan
> >>>>>>
> >>>>>>
> >>>>>> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com>
> >>>>> wrote:
> >>>>>>
> >>>>>>> Daan,
> >>>>>>>
> >>>>>>> I just looked through the review request, and published my
> comments.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> -John
> >>>>>>>
> >>>>>>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <
> daan.hoogland@gmail.com
> >>>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Hiroaki,
> >>>>>>>>
> >>>>>>>> - auto-fix may happen where it is really required
> >>>>>>>>>
> >>>>>>>> I do not have a clear view on this, so I took the approach of
> better
> >>>>> safe
> >>>>>>>> then sorry. The submitted is what works. I don't see how the
> >> auto-fix
> >>>>>>>> should ever be needed if the source is fixed. Hope you can live
> with
> >>>>>>> this.
> >>>>>>>>
> >>>>>>>>> - and if auto-fix happens, it should log it with
> >>>>>>>>> WARN level.
> >>>>>>>>
> >>>>>>>> Applied
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> regards,
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <
> >>>>> daan.hoogland@gmail.com
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> Thanks Hiroaki,
> >>>>>>>>>
> >>>>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
> >>>>>>> kawai@stratosphere.co.jp>wrote:
> >>>>>>>>>
> >>>>>>>>>> I'd suggest:
> >>>>>>>>>> - fix the generation of double slash itself
> >>>>>>>>>>
> >>>>>>>>> Is in the patch
> >>>>>>>>>
> >>>>>>>>>> - auto-fix may happen where it is really required
> >>>>>>>>>> - and if auto-fix happens, it should log it with
> >>>>>>>>>> WARN level.
> >>>>>>>>>
> >>>>>>>>> Good point, I will up the level in an update.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> (2013/06/13 21:15), Daan Hoogland wrote:
> >>>>>>>>>>
> >>>>>>>>>>> H,
> >>>>>>>>>>>
> >>>>>>>>>>> Can someone look at Review Request #11861<
> >> https://reviews.apache.**
> >>>>>>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me
> >> please?
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks,
> >>>>>>>>>>> Daan Hoogland
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >>
>
>
Re: committer wanted for review
Posted by John Burwell <jb...@basho.com>.
Daan,
Please see my comments in-line below.
Thanks,
-John
On Jun 17, 2013, at 9:40 AM, Daan Hoogland <da...@gmail.com> wrote:
> John,
>
> If I understand it correctly, you are stating that my take on the solution
> is 'not done/not the way to go'?
>
> For the record the case I solved was an instance of A, but I would not call
> it adding technical debt. A arose from existing code in combination of a
> requirement to work with a non-posix-path compliant (but unc) nfs server.
From my perspective, it is technical debt because the solution, as implemented, is masking/compensating for underlying defects. I think we should fix the underlying defects, input validation and value persistence, rather than trying to compensate for it in the storage layer. We also likely need some type of utility/functionality to upgrade tools to identify invalid path data in existing installations for correction.
>
> regards,
>
>
> On Mon, Jun 17, 2013 at 2:01 PM, John Burwell <jb...@basho.com> wrote:
>
>> All,
>>
>> Please see my comments in-line below.
>>
>> Thanks,
>> -John
>>
>> On Jun 15, 2013, at 6:11 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp>
>> wrote:
>>
>>> Probably we've agreed on that double slash should not
>>> generated by cloudstack.
>>>
>>> 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.,
>>
>> A indicates that we adding technical debt and later defects to the system.
>> We need to fix upstream for correctness before it rots further. B sound
>> like a case for stronger input validation rather than a "fix up" on the
>> backend. C seems like we need to be more careful in how we persist and
>> retrieve the information from the database. The more we discuss this
>> solution, the more this feels like a front-end input validation and
>> database persistence issue. Treating it this way would obviate any
>> security issues or logging needs.
>>
>>>
>>> Anyway, double slash should not happen and the admins should be
>>> able to know when the NFS layer got that sequence.
>>> I'd prefer WARN for this reason, but INFO may do as well.
>>> I don't have strong opinion on log level.
>>
>>
>> If it shouldn't happen then we should be rejecting the data as part of
>> input validation and no allowing it to be persisted.
>>
>>>
>>> In addition to that, "auto-fix" may not be a "fix" for example in
>>> case "C". I don't want to see autofix code in many places,
>>> "auto-fix" might be a "fix" where the path is really passed to
>>> NFS layer.
>>>
>>> Another approach to double-slash is just reject the input and raise
>>> a CloudstackRuntimeException.
>>> But I'd prefer auto-fix because of case "A" at this moment…
>>
>> Originally, I thought this fix was the equivalent of escaping a URL or
>> HTML string. Now that I understand it more fully, I believe we need to
>> throw a CloudRuntimeException to ferret out code generating incorrectly
>> formatted input.
>>
>>>
>>>
>>> (2013/06/15 18:01), Daan Hoogland wrote:
>>>> H John,
>>>>
>>>> Yes, actually I was going to make it info level but you swapped me of my
>>>> feet with your remark.
>>>>
>>>> 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, does not destabalize cloudstack in any
>>>> way, so I will stick with the info. It is certainly not debug in my
>>>> opinion. It is not a bug that needs debugging.
>>>>
>>>> Of course a deeper understanding of cloudstack might change my position
>> on
>>>> the issue.
>>>>
>>>> regards,
>>>> Daan
>>>>
>>>>
>>>> On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jb...@basho.com>
>> wrote:
>>>>
>>>>> Daan,
>>>>>
>>>>> Since a WARN indicates a condition that could lead to system
>> instability,
>>>>> many folks configure their log analysis to trigger notifications on
>> WARN
>>>>> and INFO. Does escaping a character in a path warrant meet that
>> criteria?
>>>>>
>>>>> Thanks,
>>>>> -John
>>>>>
>>>>> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <da...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> H John,
>>>>>>
>>>>>> I browsed through your comments and most I will apply. There is one
>> where
>>>>>> you contradict Hiroaki. This is about the logging level for reporting
>> a
>>>>>> changed path. I am going to follow my heart at this unless there is a
>>>>>> project directive on it.
>>>>>>
>>>>>> regards,
>>>>>> Daan
>>>>>>
>>>>>>
>>>>>> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com>
>>>>> wrote:
>>>>>>
>>>>>>> Daan,
>>>>>>>
>>>>>>> I just looked through the review request, and published my comments.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> -John
>>>>>>>
>>>>>>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <daan.hoogland@gmail.com
>>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Hiroaki,
>>>>>>>>
>>>>>>>> - auto-fix may happen where it is really required
>>>>>>>>>
>>>>>>>> I do not have a clear view on this, so I took the approach of better
>>>>> safe
>>>>>>>> then sorry. The submitted is what works. I don't see how the
>> auto-fix
>>>>>>>> should ever be needed if the source is fixed. Hope you can live with
>>>>>>> this.
>>>>>>>>
>>>>>>>>> - and if auto-fix happens, it should log it with
>>>>>>>>> WARN level.
>>>>>>>>
>>>>>>>> Applied
>>>>>>>>
>>>>>>>>
>>>>>>>> regards,
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <
>>>>> daan.hoogland@gmail.com
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Thanks Hiroaki,
>>>>>>>>>
>>>>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
>>>>>>> kawai@stratosphere.co.jp>wrote:
>>>>>>>>>
>>>>>>>>>> I'd suggest:
>>>>>>>>>> - fix the generation of double slash itself
>>>>>>>>>>
>>>>>>>>> Is in the patch
>>>>>>>>>
>>>>>>>>>> - auto-fix may happen where it is really required
>>>>>>>>>> - and if auto-fix happens, it should log it with
>>>>>>>>>> WARN level.
>>>>>>>>>
>>>>>>>>> Good point, I will up the level in an update.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> (2013/06/13 21:15), Daan Hoogland wrote:
>>>>>>>>>>
>>>>>>>>>>> H,
>>>>>>>>>>>
>>>>>>>>>>> Can someone look at Review Request #11861<
>> https://reviews.apache.**
>>>>>>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me
>> please?
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Daan Hoogland
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
Re: committer wanted for review
Posted by Daan Hoogland <da...@gmail.com>.
John,
If I understand it correctly, you are stating that my take on the solution
is 'not done/not the way to go'?
For the record the case I solved was an instance of A, but I would not call
it adding technical debt. A arose from existing code in combination of a
requirement to work with a non-posix-path compliant (but unc) nfs server.
regards,
On Mon, Jun 17, 2013 at 2:01 PM, John Burwell <jb...@basho.com> wrote:
> All,
>
> Please see my comments in-line below.
>
> Thanks,
> -John
>
> On Jun 15, 2013, at 6:11 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp>
> wrote:
>
> > Probably we've agreed on that double slash should not
> > generated by cloudstack.
> >
> > 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.,
>
> A indicates that we adding technical debt and later defects to the system.
> We need to fix upstream for correctness before it rots further. B sound
> like a case for stronger input validation rather than a "fix up" on the
> backend. C seems like we need to be more careful in how we persist and
> retrieve the information from the database. The more we discuss this
> solution, the more this feels like a front-end input validation and
> database persistence issue. Treating it this way would obviate any
> security issues or logging needs.
>
> >
> > Anyway, double slash should not happen and the admins should be
> > able to know when the NFS layer got that sequence.
> > I'd prefer WARN for this reason, but INFO may do as well.
> > I don't have strong opinion on log level.
>
>
> If it shouldn't happen then we should be rejecting the data as part of
> input validation and no allowing it to be persisted.
>
> >
> > In addition to that, "auto-fix" may not be a "fix" for example in
> > case "C". I don't want to see autofix code in many places,
> > "auto-fix" might be a "fix" where the path is really passed to
> > NFS layer.
> >
> > Another approach to double-slash is just reject the input and raise
> > a CloudstackRuntimeException.
> > But I'd prefer auto-fix because of case "A" at this moment…
>
> Originally, I thought this fix was the equivalent of escaping a URL or
> HTML string. Now that I understand it more fully, I believe we need to
> throw a CloudRuntimeException to ferret out code generating incorrectly
> formatted input.
>
> >
> >
> > (2013/06/15 18:01), Daan Hoogland wrote:
> >> H John,
> >>
> >> Yes, actually I was going to make it info level but you swapped me of my
> >> feet with your remark.
> >>
> >> 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, does not destabalize cloudstack in any
> >> way, so I will stick with the info. It is certainly not debug in my
> >> opinion. It is not a bug that needs debugging.
> >>
> >> Of course a deeper understanding of cloudstack might change my position
> on
> >> the issue.
> >>
> >> regards,
> >> Daan
> >>
> >>
> >> On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jb...@basho.com>
> wrote:
> >>
> >>> Daan,
> >>>
> >>> Since a WARN indicates a condition that could lead to system
> instability,
> >>> many folks configure their log analysis to trigger notifications on
> WARN
> >>> and INFO. Does escaping a character in a path warrant meet that
> criteria?
> >>>
> >>> Thanks,
> >>> -John
> >>>
> >>> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <da...@gmail.com>
> >>> wrote:
> >>>
> >>>> H John,
> >>>>
> >>>> I browsed through your comments and most I will apply. There is one
> where
> >>>> you contradict Hiroaki. This is about the logging level for reporting
> a
> >>>> changed path. I am going to follow my heart at this unless there is a
> >>>> project directive on it.
> >>>>
> >>>> regards,
> >>>> Daan
> >>>>
> >>>>
> >>>> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com>
> >>> wrote:
> >>>>
> >>>>> Daan,
> >>>>>
> >>>>> I just looked through the review request, and published my comments.
> >>>>>
> >>>>> Thanks,
> >>>>> -John
> >>>>>
> >>>>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <daan.hoogland@gmail.com
> >
> >>>>> wrote:
> >>>>>
> >>>>>> Hiroaki,
> >>>>>>
> >>>>>> - auto-fix may happen where it is really required
> >>>>>>>
> >>>>>> I do not have a clear view on this, so I took the approach of better
> >>> safe
> >>>>>> then sorry. The submitted is what works. I don't see how the
> auto-fix
> >>>>>> should ever be needed if the source is fixed. Hope you can live with
> >>>>> this.
> >>>>>>
> >>>>>>> - and if auto-fix happens, it should log it with
> >>>>>>> WARN level.
> >>>>>>
> >>>>>> Applied
> >>>>>>
> >>>>>>
> >>>>>> regards,
> >>>>>>
> >>>>>>
> >>>>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <
> >>> daan.hoogland@gmail.com
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Thanks Hiroaki,
> >>>>>>>
> >>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
> >>>>> kawai@stratosphere.co.jp>wrote:
> >>>>>>>
> >>>>>>>> I'd suggest:
> >>>>>>>> - fix the generation of double slash itself
> >>>>>>>>
> >>>>>>> Is in the patch
> >>>>>>>
> >>>>>>>> - auto-fix may happen where it is really required
> >>>>>>>> - and if auto-fix happens, it should log it with
> >>>>>>>> WARN level.
> >>>>>>>
> >>>>>>> Good point, I will up the level in an update.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> (2013/06/13 21:15), Daan Hoogland wrote:
> >>>>>>>>
> >>>>>>>>> H,
> >>>>>>>>>
> >>>>>>>>> Can someone look at Review Request #11861<
> https://reviews.apache.**
> >>>>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me
> please?
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>> Daan Hoogland
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>
> >
>
>
Re: committer wanted for review
Posted by John Burwell <jb...@basho.com>.
All,
Please see my comments in-line below.
Thanks,
-John
On Jun 15, 2013, at 6:11 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp> wrote:
> Probably we've agreed on that double slash should not
> generated by cloudstack.
>
> 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.,
A indicates that we adding technical debt and later defects to the system. We need to fix upstream for correctness before it rots further. B sound like a case for stronger input validation rather than a "fix up" on the backend. C seems like we need to be more careful in how we persist and retrieve the information from the database. The more we discuss this solution, the more this feels like a front-end input validation and database persistence issue. Treating it this way would obviate any security issues or logging needs.
>
> Anyway, double slash should not happen and the admins should be
> able to know when the NFS layer got that sequence.
> I'd prefer WARN for this reason, but INFO may do as well.
> I don't have strong opinion on log level.
If it shouldn't happen then we should be rejecting the data as part of input validation and no allowing it to be persisted.
>
> In addition to that, "auto-fix" may not be a "fix" for example in
> case "C". I don't want to see autofix code in many places,
> "auto-fix" might be a "fix" where the path is really passed to
> NFS layer.
>
> Another approach to double-slash is just reject the input and raise
> a CloudstackRuntimeException.
> But I'd prefer auto-fix because of case "A" at this moment…
Originally, I thought this fix was the equivalent of escaping a URL or HTML string. Now that I understand it more fully, I believe we need to throw a CloudRuntimeException to ferret out code generating incorrectly formatted input.
>
>
> (2013/06/15 18:01), Daan Hoogland wrote:
>> H John,
>>
>> Yes, actually I was going to make it info level but you swapped me of my
>> feet with your remark.
>>
>> 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, does not destabalize cloudstack in any
>> way, so I will stick with the info. It is certainly not debug in my
>> opinion. It is not a bug that needs debugging.
>>
>> Of course a deeper understanding of cloudstack might change my position on
>> the issue.
>>
>> regards,
>> Daan
>>
>>
>> On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jb...@basho.com> wrote:
>>
>>> Daan,
>>>
>>> Since a WARN indicates a condition that could lead to system instability,
>>> many folks configure their log analysis to trigger notifications on WARN
>>> and INFO. Does escaping a character in a path warrant meet that criteria?
>>>
>>> Thanks,
>>> -John
>>>
>>> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <da...@gmail.com>
>>> wrote:
>>>
>>>> H John,
>>>>
>>>> I browsed through your comments and most I will apply. There is one where
>>>> you contradict Hiroaki. This is about the logging level for reporting a
>>>> changed path. I am going to follow my heart at this unless there is a
>>>> project directive on it.
>>>>
>>>> regards,
>>>> Daan
>>>>
>>>>
>>>> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com>
>>> wrote:
>>>>
>>>>> Daan,
>>>>>
>>>>> I just looked through the review request, and published my comments.
>>>>>
>>>>> Thanks,
>>>>> -John
>>>>>
>>>>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <da...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hiroaki,
>>>>>>
>>>>>> - auto-fix may happen where it is really required
>>>>>>>
>>>>>> I do not have a clear view on this, so I took the approach of better
>>> safe
>>>>>> then sorry. The submitted is what works. I don't see how the auto-fix
>>>>>> should ever be needed if the source is fixed. Hope you can live with
>>>>> this.
>>>>>>
>>>>>>> - and if auto-fix happens, it should log it with
>>>>>>> WARN level.
>>>>>>
>>>>>> Applied
>>>>>>
>>>>>>
>>>>>> regards,
>>>>>>
>>>>>>
>>>>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <
>>> daan.hoogland@gmail.com
>>>>>> wrote:
>>>>>>
>>>>>>> Thanks Hiroaki,
>>>>>>>
>>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
>>>>> kawai@stratosphere.co.jp>wrote:
>>>>>>>
>>>>>>>> I'd suggest:
>>>>>>>> - fix the generation of double slash itself
>>>>>>>>
>>>>>>> Is in the patch
>>>>>>>
>>>>>>>> - auto-fix may happen where it is really required
>>>>>>>> - and if auto-fix happens, it should log it with
>>>>>>>> WARN level.
>>>>>>>
>>>>>>> Good point, I will up the level in an update.
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> (2013/06/13 21:15), Daan Hoogland wrote:
>>>>>>>>
>>>>>>>>> H,
>>>>>>>>>
>>>>>>>>> Can someone look at Review Request #11861<https://reviews.apache.**
>>>>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Daan Hoogland
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>
Re: committer wanted for review
Posted by Hiroaki KAWAI <ka...@stratosphere.co.jp>.
Probably we've agreed on that double slash should not
generated by cloudstack.
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.,
Anyway, double slash should not happen and the admins should be
able to know when the NFS layer got that sequence.
I'd prefer WARN for this reason, but INFO may do as well.
I don't have strong opinion on log level.
In addition to that, "auto-fix" may not be a "fix" for example in
case "C". I don't want to see autofix code in many places,
"auto-fix" might be a "fix" where the path is really passed to
NFS layer.
Another approach to double-slash is just reject the input and raise
a CloudstackRuntimeException.
But I'd prefer auto-fix because of case "A" at this moment...
(2013/06/15 18:01), Daan Hoogland wrote:
> H John,
>
> Yes, actually I was going to make it info level but you swapped me of my
> feet with your remark.
>
> 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, does not destabalize cloudstack in any
> way, so I will stick with the info. It is certainly not debug in my
> opinion. It is not a bug that needs debugging.
>
> Of course a deeper understanding of cloudstack might change my position on
> the issue.
>
> regards,
> Daan
>
>
> On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jb...@basho.com> wrote:
>
>> Daan,
>>
>> Since a WARN indicates a condition that could lead to system instability,
>> many folks configure their log analysis to trigger notifications on WARN
>> and INFO. Does escaping a character in a path warrant meet that criteria?
>>
>> Thanks,
>> -John
>>
>> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <da...@gmail.com>
>> wrote:
>>
>>> H John,
>>>
>>> I browsed through your comments and most I will apply. There is one where
>>> you contradict Hiroaki. This is about the logging level for reporting a
>>> changed path. I am going to follow my heart at this unless there is a
>>> project directive on it.
>>>
>>> regards,
>>> Daan
>>>
>>>
>>> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com>
>> wrote:
>>>
>>>> Daan,
>>>>
>>>> I just looked through the review request, and published my comments.
>>>>
>>>> Thanks,
>>>> -John
>>>>
>>>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <da...@gmail.com>
>>>> wrote:
>>>>
>>>>> Hiroaki,
>>>>>
>>>>> - auto-fix may happen where it is really required
>>>>>>
>>>>> I do not have a clear view on this, so I took the approach of better
>> safe
>>>>> then sorry. The submitted is what works. I don't see how the auto-fix
>>>>> should ever be needed if the source is fixed. Hope you can live with
>>>> this.
>>>>>
>>>>>> - and if auto-fix happens, it should log it with
>>>>>> WARN level.
>>>>>
>>>>> Applied
>>>>>
>>>>>
>>>>> regards,
>>>>>
>>>>>
>>>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <
>> daan.hoogland@gmail.com
>>>>> wrote:
>>>>>
>>>>>> Thanks Hiroaki,
>>>>>>
>>>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
>>>> kawai@stratosphere.co.jp>wrote:
>>>>>>
>>>>>>> I'd suggest:
>>>>>>> - fix the generation of double slash itself
>>>>>>>
>>>>>> Is in the patch
>>>>>>
>>>>>>> - auto-fix may happen where it is really required
>>>>>>> - and if auto-fix happens, it should log it with
>>>>>>> WARN level.
>>>>>>
>>>>>> Good point, I will up the level in an update.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> (2013/06/13 21:15), Daan Hoogland wrote:
>>>>>>>
>>>>>>>> H,
>>>>>>>>
>>>>>>>> Can someone look at Review Request #11861<https://reviews.apache.**
>>>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Daan Hoogland
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>
Re: committer wanted for review
Posted by Daan Hoogland <da...@gmail.com>.
H John,
Yes, actually I was going to make it info level but you swapped me of my
feet with your remark.
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, does not destabalize cloudstack in any
way, so I will stick with the info. It is certainly not debug in my
opinion. It is not a bug that needs debugging.
Of course a deeper understanding of cloudstack might change my position on
the issue.
regards,
Daan
On Fri, Jun 14, 2013 at 5:58 PM, John Burwell <jb...@basho.com> wrote:
> Daan,
>
> Since a WARN indicates a condition that could lead to system instability,
> many folks configure their log analysis to trigger notifications on WARN
> and INFO. Does escaping a character in a path warrant meet that criteria?
>
> Thanks,
> -John
>
> On Jun 14, 2013, at 11:52 AM, Daan Hoogland <da...@gmail.com>
> wrote:
>
> > H John,
> >
> > I browsed through your comments and most I will apply. There is one where
> > you contradict Hiroaki. This is about the logging level for reporting a
> > changed path. I am going to follow my heart at this unless there is a
> > project directive on it.
> >
> > regards,
> > Daan
> >
> >
> > On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com>
> wrote:
> >
> >> Daan,
> >>
> >> I just looked through the review request, and published my comments.
> >>
> >> Thanks,
> >> -John
> >>
> >> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <da...@gmail.com>
> >> wrote:
> >>
> >>> Hiroaki,
> >>>
> >>> - auto-fix may happen where it is really required
> >>>>
> >>> I do not have a clear view on this, so I took the approach of better
> safe
> >>> then sorry. The submitted is what works. I don't see how the auto-fix
> >>> should ever be needed if the source is fixed. Hope you can live with
> >> this.
> >>>
> >>>> - and if auto-fix happens, it should log it with
> >>>> WARN level.
> >>>
> >>> Applied
> >>>
> >>>
> >>> regards,
> >>>
> >>>
> >>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <
> daan.hoogland@gmail.com
> >>> wrote:
> >>>
> >>>> Thanks Hiroaki,
> >>>>
> >>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
> >> kawai@stratosphere.co.jp>wrote:
> >>>>
> >>>>> I'd suggest:
> >>>>> - fix the generation of double slash itself
> >>>>>
> >>>> Is in the patch
> >>>>
> >>>>> - auto-fix may happen where it is really required
> >>>>> - and if auto-fix happens, it should log it with
> >>>>> WARN level.
> >>>>
> >>>> Good point, I will up the level in an update.
> >>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> (2013/06/13 21:15), Daan Hoogland wrote:
> >>>>>
> >>>>>> H,
> >>>>>>
> >>>>>> Can someone look at Review Request #11861<https://reviews.apache.**
> >>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Daan Hoogland
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> >>
>
>
Re: committer wanted for review
Posted by John Burwell <jb...@basho.com>.
Daan,
Since a WARN indicates a condition that could lead to system instability, many folks configure their log analysis to trigger notifications on WARN and INFO. Does escaping a character in a path warrant meet that criteria?
Thanks,
-John
On Jun 14, 2013, at 11:52 AM, Daan Hoogland <da...@gmail.com> wrote:
> H John,
>
> I browsed through your comments and most I will apply. There is one where
> you contradict Hiroaki. This is about the logging level for reporting a
> changed path. I am going to follow my heart at this unless there is a
> project directive on it.
>
> regards,
> Daan
>
>
> On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com> wrote:
>
>> Daan,
>>
>> I just looked through the review request, and published my comments.
>>
>> Thanks,
>> -John
>>
>> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <da...@gmail.com>
>> wrote:
>>
>>> Hiroaki,
>>>
>>> - auto-fix may happen where it is really required
>>>>
>>> I do not have a clear view on this, so I took the approach of better safe
>>> then sorry. The submitted is what works. I don't see how the auto-fix
>>> should ever be needed if the source is fixed. Hope you can live with
>> this.
>>>
>>>> - and if auto-fix happens, it should log it with
>>>> WARN level.
>>>
>>> Applied
>>>
>>>
>>> regards,
>>>
>>>
>>> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <daan.hoogland@gmail.com
>>> wrote:
>>>
>>>> Thanks Hiroaki,
>>>>
>>>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
>> kawai@stratosphere.co.jp>wrote:
>>>>
>>>>> I'd suggest:
>>>>> - fix the generation of double slash itself
>>>>>
>>>> Is in the patch
>>>>
>>>>> - auto-fix may happen where it is really required
>>>>> - and if auto-fix happens, it should log it with
>>>>> WARN level.
>>>>
>>>> Good point, I will up the level in an update.
>>>>
>>>>>
>>>>>
>>>>>
>>>>> (2013/06/13 21:15), Daan Hoogland wrote:
>>>>>
>>>>>> H,
>>>>>>
>>>>>> Can someone look at Review Request #11861<https://reviews.apache.**
>>>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
>>>>>>
>>>>>> Thanks,
>>>>>> Daan Hoogland
>>>>>>
>>>>>>
>>>>>
>>>>
>>
>>
Re: committer wanted for review
Posted by Daan Hoogland <da...@gmail.com>.
H John,
I browsed through your comments and most I will apply. There is one where
you contradict Hiroaki. This is about the logging level for reporting a
changed path. I am going to follow my heart at this unless there is a
project directive on it.
regards,
Daan
On Fri, Jun 14, 2013 at 5:25 PM, John Burwell <jb...@basho.com> wrote:
> Daan,
>
> I just looked through the review request, and published my comments.
>
> Thanks,
> -John
>
> On Jun 14, 2013, at 10:27 AM, Daan Hoogland <da...@gmail.com>
> wrote:
>
> > Hiroaki,
> >
> > - auto-fix may happen where it is really required
> >>
> > I do not have a clear view on this, so I took the approach of better safe
> > then sorry. The submitted is what works. I don't see how the auto-fix
> > should ever be needed if the source is fixed. Hope you can live with
> this.
> >
> >> - and if auto-fix happens, it should log it with
> >> WARN level.
> >
> > Applied
> >
> >
> > regards,
> >
> >
> > On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <daan.hoogland@gmail.com
> >wrote:
> >
> >> Thanks Hiroaki,
> >>
> >> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <
> kawai@stratosphere.co.jp>wrote:
> >>
> >>> I'd suggest:
> >>> - fix the generation of double slash itself
> >>>
> >> Is in the patch
> >>
> >>> - auto-fix may happen where it is really required
> >>> - and if auto-fix happens, it should log it with
> >>> WARN level.
> >>
> >> Good point, I will up the level in an update.
> >>
> >>>
> >>>
> >>>
> >>> (2013/06/13 21:15), Daan Hoogland wrote:
> >>>
> >>>> H,
> >>>>
> >>>> Can someone look at Review Request #11861<https://reviews.apache.**
> >>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
> >>>>
> >>>> Thanks,
> >>>> Daan Hoogland
> >>>>
> >>>>
> >>>
> >>
>
>
Re: committer wanted for review
Posted by John Burwell <jb...@basho.com>.
Daan,
I just looked through the review request, and published my comments.
Thanks,
-John
On Jun 14, 2013, at 10:27 AM, Daan Hoogland <da...@gmail.com> wrote:
> Hiroaki,
>
> - auto-fix may happen where it is really required
>>
> I do not have a clear view on this, so I took the approach of better safe
> then sorry. The submitted is what works. I don't see how the auto-fix
> should ever be needed if the source is fixed. Hope you can live with this.
>
>> - and if auto-fix happens, it should log it with
>> WARN level.
>
> Applied
>
>
> regards,
>
>
> On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <da...@gmail.com>wrote:
>
>> Thanks Hiroaki,
>>
>> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp>wrote:
>>
>>> I'd suggest:
>>> - fix the generation of double slash itself
>>>
>> Is in the patch
>>
>>> - auto-fix may happen where it is really required
>>> - and if auto-fix happens, it should log it with
>>> WARN level.
>>
>> Good point, I will up the level in an update.
>>
>>>
>>>
>>>
>>> (2013/06/13 21:15), Daan Hoogland wrote:
>>>
>>>> H,
>>>>
>>>> Can someone look at Review Request #11861<https://reviews.apache.**
>>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
>>>>
>>>> Thanks,
>>>> Daan Hoogland
>>>>
>>>>
>>>
>>
Re: committer wanted for review
Posted by Daan Hoogland <da...@gmail.com>.
Hiroaki,
- auto-fix may happen where it is really required
>
I do not have a clear view on this, so I took the approach of better safe
then sorry. The submitted is what works. I don't see how the auto-fix
should ever be needed if the source is fixed. Hope you can live with this.
> - and if auto-fix happens, it should log it with
> WARN level.
Applied
regards,
On Fri, Jun 14, 2013 at 10:35 AM, Daan Hoogland <da...@gmail.com>wrote:
> Thanks Hiroaki,
>
> On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp>wrote:
>
>> I'd suggest:
>> - fix the generation of double slash itself
>>
> Is in the patch
>
>> - auto-fix may happen where it is really required
>> - and if auto-fix happens, it should log it with
>> WARN level.
>
> Good point, I will up the level in an update.
>
>>
>>
>>
>> (2013/06/13 21:15), Daan Hoogland wrote:
>>
>>> H,
>>>
>>> Can someone look at Review Request #11861<https://reviews.apache.**
>>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
>>>
>>> Thanks,
>>> Daan Hoogland
>>>
>>>
>>
>
Re: committer wanted for review
Posted by Daan Hoogland <da...@gmail.com>.
Thanks Hiroaki,
On Fri, Jun 14, 2013 at 3:41 AM, Hiroaki KAWAI <ka...@stratosphere.co.jp>wrote:
> I'd suggest:
> - fix the generation of double slash itself
>
Is in the patch
> - auto-fix may happen where it is really required
> - and if auto-fix happens, it should log it with
> WARN level.
Good point, I will up the level in an update.
>
>
>
> (2013/06/13 21:15), Daan Hoogland wrote:
>
>> H,
>>
>> Can someone look at Review Request #11861<https://reviews.apache.**
>> org/r/11861/ <https://reviews.apache.org/r/11861/>> for me please?
>>
>> Thanks,
>> Daan Hoogland
>>
>>
>
Re: committer wanted for review
Posted by Hiroaki KAWAI <ka...@stratosphere.co.jp>.
I'd suggest:
- fix the generation of double slash itself
- auto-fix may happen where it is really required
- and if auto-fix happens, it should log it with
WARN level.
(2013/06/13 21:15), Daan Hoogland wrote:
> H,
>
> Can someone look at Review Request #11861<https://reviews.apache.org/r/11861/> for me please?
>
> Thanks,
> Daan Hoogland
>
RE: committer wanted for review
Posted by Daan Hoogland <DH...@schubergphilis.com>.
Ilya,
Why do you ask? It only touches the database, not the nfs server itself!
Regards,
Daan Hoogland
-----Original Message-----
From: Daan Hoogland [mailto:DHoogland@schubergphilis.com]
Sent: donderdag 13 juni 2013 15:32
To: 'dev@cloudstack.apache.org'
Subject: RE: committer wanted for review
It has been tested on nexenta (sunos). I have no linux on my hands for this.
-----Original Message-----
From: Musayev, Ilya [mailto:imusayev@webmd.net]
Sent: donderdag 13 juni 2013 15:27
To: dev@cloudstack.apache.org
Subject: Re: committer wanted for review
Daan,
Has this been tested on linux NFS?
On 6/13/13 8:15 AM, "Daan Hoogland" <DH...@schubergphilis.com> wrote:
>H,
>
>Can someone look at Review Request
>#11861<https://reviews.apache.org/r/11861/> for me please?
>
>Thanks,
>Daan Hoogland
RE: committer wanted for review
Posted by Daan Hoogland <DH...@schubergphilis.com>.
It has been tested on nexenta (sunos). I have no linux on my hands for this.
-----Original Message-----
From: Musayev, Ilya [mailto:imusayev@webmd.net]
Sent: donderdag 13 juni 2013 15:27
To: dev@cloudstack.apache.org
Subject: Re: committer wanted for review
Daan,
Has this been tested on linux NFS?
On 6/13/13 8:15 AM, "Daan Hoogland" <DH...@schubergphilis.com> wrote:
>H,
>
>Can someone look at Review Request
>#11861<https://reviews.apache.org/r/11861/> for me please?
>
>Thanks,
>Daan Hoogland
Re: committer wanted for review
Posted by "Musayev, Ilya" <im...@webmd.net>.
Daan,
Has this been tested on linux NFS?
On 6/13/13 8:15 AM, "Daan Hoogland" <DH...@schubergphilis.com> wrote:
>H,
>
>Can someone look at Review Request
>#11861<https://reviews.apache.org/r/11861/> for me please?
>
>Thanks,
>Daan Hoogland