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