[Engine-devel] LOCALFS path validation

[Itamar Heim]

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