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 <jburw...@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.hoogl...@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 <jburw...@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.hoogl...@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.hoogl...@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 >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>> >>>>> >>> >>> >> >
