Re: [Engine-devel] LOCALFS path validation

On 06/03/2012 05:04 AM, Itamar Heim wrote: > On 06/01/2012 08:16 PM, Amador Pahim wrote: >> On 06/01/2012 12:39 PM, Itamar Heim wrote: >>> On 05/21/2012 09:37 PM, Gilad Chaplik wrote: >>>> Hi Pahim, >>>> >>>> Thanks for the input! >>>> Comments inline. >>>> >>>> Thanks, >>>> Gilad. >>>> >>>> ----- Original Message ----- >>>>> From: "Amador pahim"<apahim(a)redhat.com> >>>>> To: engine-devel(a)ovirt.org >>>>> Sent: Monday, May 21, 2012 5:52:34 PM >>>>> Subject: [Engine-devel] LOCALFS path validation >>>>> >>>>> >>>>> Hello, >>>>> >>>>> I'm starting to know the engine code. I chose a little unstandardized >>>>> behaviour to follow through the devel process. I have a patch and >>>>> I'd like to know if you fell relevant to correct this issue: >>>>> >>>>> - Description: Adding a LOCAL storage [1], webadmin does not validate >>>>> path against regex, sendind the invalid path (with final slash) to >>>>> vdsm [2] [3]. But, adding a NFS storage, the path is validated >>>>> before contacting vdsm [4] avoiding extra vdsm processing and >>>>> quickly/clearly informing user about what's wrong. >>>>> >>>>> - Expected result: Same behaviour to NFS and LOCALFS storage path >>>>> validation. Validate LOCALFS path in webadmin before send it to vdsm >>>>> [5]. >>>> >>>> you may and should send a patch :) >>>> >>>>> >>>>> - Newbie doubt: Wouldn't be better to validate the both local and nfs >>>>> path on the backend, achieving all user interfaces/APIs? >>>> >>>> Because we have a rich client app (gwt), we can perform the >>>> validation also in the client side very easily, >>>> we do that to avoid unnecessary calls to the backend side, and to >>>> have a better& responsive ui >>>> (client side validation is performed instantly - without the need to >>>> wait). >>>> >>>> Anyway, every validation performed in the client side needs to be >>>> performed also in backend side (for api, and other reasons >>>> (security?)). >>> >>> the client side can validate the format of the path, not if it really >>> exists. >>> do you mean local storage via the wizard, or just adding another >>> storage domain? >>> the 'configure local storage' wizard requires the host to be in >>> maintenance, which may or may not be a problem to implement this type >>> of validation. >> >> I mean just adding a LOCALFS storage domain with an invalid path, like >> this: >> https://picasaweb.google.com/lh/photo/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj3f... >> >> >> Currently we have this: >> https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3f... >> >> >> I sent a patch... >> http://gerrit.ovirt.org/4857 >> ... in order to do this: >> https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3f... >> >> >> Just like "New NFS Domain" currently does: >> https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3f... >> >> > > ok - sounds basic enough of a change. I'd just note in the patch you > are re-using same REGEX used for the previous validation happening in > the backend (and actually try to re-use it, to not maintain a change > in too many places0 There is no validation in backend. I mean, the error "Error while executing action AddLocalStorageDomain: Remote path is illegal" is coming from VDSM: https://gist.github.com/2762656 And is not caused by regex. VDSM is just comparing path /test/ with os.path.abspath(), /test: https://gist.github.com/2867874