[Engine-devel] LOCALFS path validation

[Amador Pahim]

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 at redhat.com>
>>>>> To: engine-devel at 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/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj3fEcMleB60 
>>
>>
>> Currently we have this:
>> https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3fEcMleB60 
>>
>>
>> I sent a patch...
>> http://gerrit.ovirt.org/4857
>> ... in order to do this:
>> https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3fEcMleB60 
>>
>>
>> Just like "New NFS Domain" currently does:
>> https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3fEcMleB60 
>>
>>
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/engine-devel/attachments/20120604/20f77306/attachment.html>