From apahim at redhat.com Mon May 21 10:52:36 2012 Content-Type: multipart/mixed; boundary="===============5009912571194723807==" MIME-Version: 1.0 From: Amador pahim To: devel at ovirt.org Subject: [Engine-devel] LOCALFS path validation Date: Mon, 21 May 2012 11:52:34 -0300 Message-ID: <4FBA56B2.8010203@redhat.com> --===============5009912571194723807== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable This is a multi-part message in MIME format. --------------040303020501050109020008 Content-Type: text/plain; charset=3DISO-8859-1; format=3Dflowed Content-Transfer-Encoding: 7bit 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]. - Newbie doubt: Wouldn't be better to validate the both local and nfs = path on the backend, achieving all user interfaces/APIs? [1] - = https://picasaweb.google.com/lh/photo/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj3fEc= MleB60 [2] - = https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3fEc= MleB60 [3] - https://gist.github.com/2762656 [4] - = https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3fEc= MleB60 [5] - = https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3fEc= MleB60 I look forward to hearing your comments. Best Regards, -- Pahim --------------040303020501050109020008 Content-Type: text/html; charset=3DISO-8859-1 Content-Transfer-Encoding: 7bit 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].

- Newbie doubt: Wouldn't be better to validate the both local and nfs path on the backend, achieving all user interfaces/APIs?

[1] - https://picasaweb.google= .com/lh/photo/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj3fEcMleB60
[2] - https://picasaweb.google= .com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3fEcMleB60
[3] - https://gist.github.com/2762656
[4] - https://picasaweb.google= .com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3fEcMleB60
[5] - https://picasaweb.google= .com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3fEcMleB60

I look forward to hearing your comments.

Best Regards,
--
Pahim
--------------040303020501050109020008-- --===============5009912571194723807== Content-Type: multipart/alternative MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="attachment.bin" VGhpcyBpcyBhIG11bHRpLXBhcnQgbWVzc2FnZSBpbiBNSU1FIGZvcm1hdC4KLS0tLS0tLS0tLS0t LS0wNDAzMDMwMjA1MDEwNTAxMDkwMjAwMDgKQ29udGVudC1UeXBlOiB0ZXh0L3BsYWluOyBjaGFy c2V0PUlTTy04ODU5LTE7IGZvcm1hdD1mbG93ZWQKQ29udGVudC1UcmFuc2Zlci1FbmNvZGluZzog N2JpdAoKSGVsbG8sCgpJJ20gc3RhcnRpbmcgdG8ga25vdyB0aGUgZW5naW5lIGNvZGUuIEkgY2hv c2UgYSBsaXR0bGUgdW5zdGFuZGFyZGl6ZWQgCmJlaGF2aW91ciB0byBmb2xsb3cgdGhyb3VnaCB0 aGUgZGV2ZWwgcHJvY2Vzcy4gSSBoYXZlIGEgcGF0Y2ggYW5kIEknZCAKbGlrZSB0byBrbm93IGlm IHlvdSBmZWxsIHJlbGV2YW50IHRvIGNvcnJlY3QgdGhpcyBpc3N1ZToKCi0gRGVzY3JpcHRpb246 IEFkZGluZyBhIExPQ0FMIHN0b3JhZ2UgWzFdLCB3ZWJhZG1pbiBkb2VzIG5vdCB2YWxpZGF0ZSAK cGF0aCBhZ2FpbnN0IHJlZ2V4LCBzZW5kaW5kIHRoZSBpbnZhbGlkIHBhdGggKHdpdGggZmluYWwg c2xhc2gpIHRvIHZkc20gClsyXSBbM10uIEJ1dCwgYWRkaW5nIGEgTkZTIHN0b3JhZ2UsIHRoZSBw YXRoIGlzIHZhbGlkYXRlZCBiZWZvcmUgCmNvbnRhY3RpbmcgdmRzbSBbNF0gYXZvaWRpbmcgZXh0 cmEgdmRzbSBwcm9jZXNzaW5nIGFuZCBxdWlja2x5L2NsZWFybHkgCmluZm9ybWluZyB1c2VyIGFi b3V0IHdoYXQncyB3cm9uZy4KCi0gRXhwZWN0ZWQgcmVzdWx0OiBTYW1lIGJlaGF2aW91ciB0byBO RlMgYW5kIExPQ0FMRlMgc3RvcmFnZSBwYXRoIAp2YWxpZGF0aW9uLiBWYWxpZGF0ZSBMT0NBTEZT IHBhdGggaW4gd2ViYWRtaW4gYmVmb3JlIHNlbmQgaXQgdG8gdmRzbSBbNV0uCgotIE5ld2JpZSBk b3VidDogV291bGRuJ3QgYmUgYmV0dGVyIHRvIHZhbGlkYXRlIHRoZSBib3RoIGxvY2FsIGFuZCBu ZnMgCnBhdGggb24gdGhlIGJhY2tlbmQsIGFjaGlldmluZyBhbGwgdXNlciBpbnRlcmZhY2VzL0FQ SXM/CgpbMV0gLSAKaHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xlLmNvbS9saC9waG90by9GV05pb3Uy WTEyR1pPM0FqZkNINks3UUF2OGNzNmVkYWozZkVjTWxlQjYwClsyXSAtIApodHRwczovL3BpY2Fz YXdlYi5nb29nbGUuY29tL2xoL3Bob3RvL1BvZjZaOG9oZ1FBa1JURHBFSktHLUxRQXY4Y3M2ZWRh ajNmRWNNbGVCNjAKWzNdIC0gaHR0cHM6Ly9naXN0LmdpdGh1Yi5jb20vMjc2MjY1NgpbNF0gLSAK aHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xlLmNvbS9saC9waG90by9GZDN6V2VnV0UwVDVDMnREb190 UFpyUUF2OGNzNmVkYWozZkVjTWxlQjYwCls1XSAtIApodHRwczovL3BpY2FzYXdlYi5nb29nbGUu Y29tL2xoL3Bob3RvL1BnellyWkhra3ZtLVd0RmtfVUZaTHJRQXY4Y3M2ZWRhajNmRWNNbGVCNjAK CkkgbG9vayBmb3J3YXJkIHRvIGhlYXJpbmcgeW91ciBjb21tZW50cy4KCkJlc3QgUmVnYXJkcywK LS0KUGFoaW0KCi0tLS0tLS0tLS0tLS0tMDQwMzAzMDIwNTAxMDUwMTA5MDIwMDA4CkNvbnRlbnQt VHlwZTogdGV4dC9odG1sOyBjaGFyc2V0PUlTTy04ODU5LTEKQ29udGVudC1UcmFuc2Zlci1FbmNv ZGluZzogN2JpdAoKPGh0bWw+CiAgPGhlYWQ+CgogICAgPG1ldGEgaHR0cC1lcXVpdj0iY29udGVu dC10eXBlIiBjb250ZW50PSJ0ZXh0L2h0bWw7IGNoYXJzZXQ9SVNPLTg4NTktMSI+CiAgPC9oZWFk PgogIDxib2R5IGJnY29sb3I9IiNGRkZGRkYiIHRleHQ9IiMwMDAwMDAiPgogICAgSGVsbG8sPGJy PgogICAgPGJyPgogICAgSSdtIHN0YXJ0aW5nIHRvIGtub3cgdGhlIGVuZ2luZSBjb2RlLiBJIGNo b3NlIGEgbGl0dGxlCiAgICB1bnN0YW5kYXJkaXplZCBiZWhhdmlvdXIgdG8gZm9sbG93IHRocm91 Z2ggdGhlIGRldmVsIHByb2Nlc3MuIEkgaGF2ZQogICAgYSBwYXRjaCBhbmQgSSdkIGxpa2UgdG8g a25vdyBpZiB5b3UgZmVsbCByZWxldmFudCB0byBjb3JyZWN0IHRoaXMKICAgIGlzc3VlOjxicj4K ICAgIDxicj4KICAgIC0gRGVzY3JpcHRpb246IEFkZGluZyBhIExPQ0FMIHN0b3JhZ2UgWzFdLCB3 ZWJhZG1pbiBkb2VzIG5vdAogICAgdmFsaWRhdGUgcGF0aCBhZ2FpbnN0IHJlZ2V4LCBzZW5kaW5k IHRoZSBpbnZhbGlkIHBhdGggKHdpdGggZmluYWwKICAgIHNsYXNoKSB0byB2ZHNtIFsyXSBbM10u IEJ1dCwgYWRkaW5nIGEgTkZTIHN0b3JhZ2UsIHRoZSBwYXRoIGlzCiAgICB2YWxpZGF0ZWQgYmVm b3JlIGNvbnRhY3RpbmcgdmRzbSBbNF0gYXZvaWRpbmcgZXh0cmEgdmRzbSBwcm9jZXNzaW5nCiAg ICBhbmQgcXVpY2tseS9jbGVhcmx5IGluZm9ybWluZyB1c2VyIGFib3V0IHdoYXQncyB3cm9uZy48 YnI+CiAgICA8YnI+CiAgICAtIEV4cGVjdGVkIHJlc3VsdDogU2FtZSBiZWhhdmlvdXIgdG8gTkZT IGFuZCBMT0NBTEZTIHN0b3JhZ2UgcGF0aAogICAgdmFsaWRhdGlvbi4gVmFsaWRhdGUgTE9DQUxG UyBwYXRoIGluIHdlYmFkbWluIGJlZm9yZSBzZW5kIGl0IHRvIHZkc20KICAgIFs1XS48YnI+CiAg ICA8YnI+CiAgICAtIE5ld2JpZSBkb3VidDogV291bGRuJ3QgYmUgYmV0dGVyIHRvIHZhbGlkYXRl IHRoZSBib3RoIGxvY2FsIGFuZAogICAgbmZzIHBhdGggb24gdGhlIGJhY2tlbmQsIGFjaGlldmlu ZyBhbGwgdXNlciBpbnRlcmZhY2VzL0FQSXM/PGJyPgogICAgPGJyPgogICAgWzFdIC0KPGEgY2xh c3M9Im1vei10eHQtbGluay1mcmVldGV4dCIgaHJlZj0iaHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xl LmNvbS9saC9waG90by9GV05pb3UyWTEyR1pPM0FqZkNINks3UUF2OGNzNmVkYWozZkVjTWxlQjYw Ij5odHRwczovL3BpY2FzYXdlYi5nb29nbGUuY29tL2xoL3Bob3RvL0ZXTmlvdTJZMTJHWk8zQWpm Q0g2SzdRQXY4Y3M2ZWRhajNmRWNNbGVCNjA8L2E+PGJyPgogICAgWzJdIC0KPGEgY2xhc3M9Im1v ei10eHQtbGluay1mcmVldGV4dCIgaHJlZj0iaHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xlLmNvbS9s aC9waG90by9Qb2Y2WjhvaGdRQWtSVERwRUpLRy1MUUF2OGNzNmVkYWozZkVjTWxlQjYwIj5odHRw czovL3BpY2FzYXdlYi5nb29nbGUuY29tL2xoL3Bob3RvL1BvZjZaOG9oZ1FBa1JURHBFSktHLUxR QXY4Y3M2ZWRhajNmRWNNbGVCNjA8L2E+PGJyPgogICAgWzNdIC0gPGEgY2xhc3M9Im1vei10eHQt bGluay1mcmVldGV4dCIgaHJlZj0iaHR0cHM6Ly9naXN0LmdpdGh1Yi5jb20vMjc2MjY1NiI+aHR0 cHM6Ly9naXN0LmdpdGh1Yi5jb20vMjc2MjY1NjwvYT48YnI+CiAgICBbNF0gLQo8YSBjbGFzcz0i bW96LXR4dC1saW5rLWZyZWV0ZXh0IiBocmVmPSJodHRwczovL3BpY2FzYXdlYi5nb29nbGUuY29t L2xoL3Bob3RvL0ZkM3pXZWdXRTBUNUMydERvX3RQWnJRQXY4Y3M2ZWRhajNmRWNNbGVCNjAiPmh0 dHBzOi8vcGljYXNhd2ViLmdvb2dsZS5jb20vbGgvcGhvdG8vRmQzeldlZ1dFMFQ1QzJ0RG9fdFBa clFBdjhjczZlZGFqM2ZFY01sZUI2MDwvYT48YnI+CiAgICBbNV0gLQo8YSBjbGFzcz0ibW96LXR4 dC1saW5rLWZyZWV0ZXh0IiBocmVmPSJodHRwczovL3BpY2FzYXdlYi5nb29nbGUuY29tL2xoL3Bo b3RvL1BnellyWkhra3ZtLVd0RmtfVUZaTHJRQXY4Y3M2ZWRhajNmRWNNbGVCNjAiPmh0dHBzOi8v cGljYXNhd2ViLmdvb2dsZS5jb20vbGgvcGhvdG8vUGd6WXJaSGtrdm0tV3RGa19VRlpMclFBdjhj czZlZGFqM2ZFY01sZUI2MDwvYT48YnI+CiAgICA8YnI+CiAgICA8bWV0YSBodHRwLWVxdWl2PSJj b250ZW50LXR5cGUiIGNvbnRlbnQ9InRleHQvaHRtbDsKICAgICAgY2hhcnNldD1JU08tODg1OS0x Ij4KICAgIEkgbG9vayBmb3J3YXJkIHRvIGhlYXJpbmcgeW91ciBjb21tZW50cy48YnI+CiAgICA8 YnI+CiAgICBCZXN0IFJlZ2FyZHMsPGJyPgogICAgLS08YnI+CiAgICBQYWhpbTxicj4KICA8L2Jv ZHk+CjwvaHRtbD4KCi0tLS0tLS0tLS0tLS0tMDQwMzAzMDIwNTAxMDUwMTA5MDIwMDA4LS0K --===============5009912571194723807==-- From gchaplik at redhat.com Mon May 21 14:37:16 2012 Content-Type: multipart/mixed; boundary="===============4280649259282094942==" MIME-Version: 1.0 From: Gilad Chaplik To: devel at ovirt.org Subject: Re: [Engine-devel] LOCALFS path validation Date: Mon, 21 May 2012 14:37:15 -0400 Message-ID: In-Reply-To: 4FBA56B2.8010203@redhat.com --===============4280649259282094942== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Pahim, Thanks for the input! Comments inline. Thanks, = Gilad. ----- Original Message ----- > From: "Amador pahim" > 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 be= tter & 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?)). > = > [1] - > https://picasaweb.google.com/lh/photo/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj3f= EcMleB60 > [2] - > https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3f= EcMleB60 > [3] - https://gist.github.com/2762656 > [4] - > https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3f= EcMleB60 > [5] - > https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3f= EcMleB60 > = > I look forward to hearing your comments. > = > Best Regards, > -- > Pahim > = > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel >=20 --===============4280649259282094942==-- From iheim at redhat.com Fri Jun 1 11:39:43 2012 Content-Type: multipart/mixed; boundary="===============6613209774968741375==" MIME-Version: 1.0 From: Itamar Heim To: devel at ovirt.org Subject: Re: [Engine-devel] LOCALFS path validation Date: Fri, 01 Jun 2012 18:39:40 +0300 Message-ID: <4FC8E23C.8060901@redhat.com> In-Reply-To: ab65230d-5b3f-4ada-9530-b46aaef2b27b@zmail14.collab.prod.int.phx2.redhat.com --===============6613209774968741375== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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" >> 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 al= so 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 perform= ed 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. --===============6613209774968741375==-- From apahim at redhat.com Fri Jun 1 15:02:41 2012 Content-Type: multipart/mixed; boundary="===============7213491723336757519==" MIME-Version: 1.0 From: Amador Pahim To: devel at ovirt.org Subject: Re: [Engine-devel] LOCALFS path validation Date: Fri, 01 Jun 2012 14:16:00 -0300 Message-ID: <4FC8F8D0.5050702@redhat.com> In-Reply-To: 4FC8E23C.8060901@redhat.com --===============7213491723336757519== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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" >>> 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/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj3fEc= MleB60 Currently we have this: https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3fEc= MleB60 I sent a patch... http://gerrit.ovirt.org/4857 ... in order to do this: https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3fEc= MleB60 Just like "New NFS Domain" currently does: https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3fEc= MleB60 --===============7213491723336757519==-- From iheim at redhat.com Sun Jun 3 04:04:39 2012 Content-Type: multipart/mixed; boundary="===============7078735160393014671==" MIME-Version: 1.0 From: Itamar Heim To: devel at ovirt.org Subject: Re: [Engine-devel] LOCALFS path validation Date: Sun, 03 Jun 2012 11:04:36 +0300 Message-ID: <4FCB1A94.8080201@redhat.com> In-Reply-To: 4FC8F8D0.5050702@redhat.com --===============7078735160393014671== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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" >>>> 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= EcMleB60 > > Currently we have this: > https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3f= EcMleB60 > > I sent a patch... > http://gerrit.ovirt.org/4857 > ... in order to do this: > https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3f= EcMleB60 > > Just like "New NFS Domain" currently does: > https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3f= EcMleB60 > 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 --===============7078735160393014671==-- From apahim at redhat.com Mon Jun 4 07:50:22 2012 Content-Type: multipart/mixed; boundary="===============8992161935189450156==" MIME-Version: 1.0 From: Amador Pahim To: devel at ovirt.org Subject: Re: [Engine-devel] LOCALFS path validation Date: Mon, 04 Jun 2012 08:50:18 -0300 Message-ID: <4FCCA0FA.9020002@redhat.com> In-Reply-To: 4FCB1A94.8080201@redhat.com --===============8992161935189450156== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable This is a multi-part message in MIME format. --------------010009060709070503080105 Content-Type: text/plain; charset=3DISO-8859-1; format=3Dflowed Content-Transfer-Encoding: 7bit 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" >>>>> 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/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj3= fEcMleB60 = >> >> >> Currently we have this: >> https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj3= fEcMleB60 = >> >> >> I sent a patch... >> http://gerrit.ovirt.org/4857 >> ... in order to do this: >> https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj3= fEcMleB60 = >> >> >> Just like "New NFS Domain" currently does: >> https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj3= fEcMleB60 = >> >> > > 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 --------------010009060709070503080105 Content-Type: text/html; charset=3DISO-8859-1 Content-Transfer-Encoding: 7bit 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/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/276= 2656
And is not caused by regex. VDSM is just comparing path /test/ with os.path.abspath(), /test:
https://gist.github.com/286= 7874
--------------010009060709070503080105-- --===============8992161935189450156== Content-Type: multipart/alternative MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="attachment.bin" VGhpcyBpcyBhIG11bHRpLXBhcnQgbWVzc2FnZSBpbiBNSU1FIGZvcm1hdC4KLS0tLS0tLS0tLS0t LS0wMTAwMDkwNjA3MDkwNzA1MDMwODAxMDUKQ29udGVudC1UeXBlOiB0ZXh0L3BsYWluOyBjaGFy c2V0PUlTTy04ODU5LTE7IGZvcm1hdD1mbG93ZWQKQ29udGVudC1UcmFuc2Zlci1FbmNvZGluZzog N2JpdAoKT24gMDYvMDMvMjAxMiAwNTowNCBBTSwgSXRhbWFyIEhlaW0gd3JvdGU6Cj4gT24gMDYv MDEvMjAxMiAwODoxNiBQTSwgQW1hZG9yIFBhaGltIHdyb3RlOgo+PiBPbiAwNi8wMS8yMDEyIDEy OjM5IFBNLCBJdGFtYXIgSGVpbSB3cm90ZToKPj4+IE9uIDA1LzIxLzIwMTIgMDk6MzcgUE0sIEdp bGFkIENoYXBsaWsgd3JvdGU6Cj4+Pj4gSGkgUGFoaW0sCj4+Pj4KPj4+PiBUaGFua3MgZm9yIHRo ZSBpbnB1dCEKPj4+PiBDb21tZW50cyBpbmxpbmUuCj4+Pj4KPj4+PiBUaGFua3MsCj4+Pj4gR2ls YWQuCj4+Pj4KPj4+PiAtLS0tLSBPcmlnaW5hbCBNZXNzYWdlIC0tLS0tCj4+Pj4+IEZyb206ICJB bWFkb3IgcGFoaW0iPGFwYWhpbUByZWRoYXQuY29tPgo+Pj4+PiBUbzogZW5naW5lLWRldmVsQG92 aXJ0Lm9yZwo+Pj4+PiBTZW50OiBNb25kYXksIE1heSAyMSwgMjAxMiA1OjUyOjM0IFBNCj4+Pj4+ IFN1YmplY3Q6IFtFbmdpbmUtZGV2ZWxdIExPQ0FMRlMgcGF0aCB2YWxpZGF0aW9uCj4+Pj4+Cj4+ Pj4+Cj4+Pj4+IEhlbGxvLAo+Pj4+Pgo+Pj4+PiBJJ20gc3RhcnRpbmcgdG8ga25vdyB0aGUgZW5n aW5lIGNvZGUuIEkgY2hvc2UgYSBsaXR0bGUgdW5zdGFuZGFyZGl6ZWQKPj4+Pj4gYmVoYXZpb3Vy IHRvIGZvbGxvdyB0aHJvdWdoIHRoZSBkZXZlbCBwcm9jZXNzLiBJIGhhdmUgYSBwYXRjaCBhbmQK Pj4+Pj4gSSdkIGxpa2UgdG8ga25vdyBpZiB5b3UgZmVsbCByZWxldmFudCB0byBjb3JyZWN0IHRo aXMgaXNzdWU6Cj4+Pj4+Cj4+Pj4+IC0gRGVzY3JpcHRpb246IEFkZGluZyBhIExPQ0FMIHN0b3Jh Z2UgWzFdLCB3ZWJhZG1pbiBkb2VzIG5vdCB2YWxpZGF0ZQo+Pj4+PiBwYXRoIGFnYWluc3QgcmVn ZXgsIHNlbmRpbmQgdGhlIGludmFsaWQgcGF0aCAod2l0aCBmaW5hbCBzbGFzaCkgdG8KPj4+Pj4g dmRzbSBbMl0gWzNdLiBCdXQsIGFkZGluZyBhIE5GUyBzdG9yYWdlLCB0aGUgcGF0aCBpcyB2YWxp ZGF0ZWQKPj4+Pj4gYmVmb3JlIGNvbnRhY3RpbmcgdmRzbSBbNF0gYXZvaWRpbmcgZXh0cmEgdmRz bSBwcm9jZXNzaW5nIGFuZAo+Pj4+PiBxdWlja2x5L2NsZWFybHkgaW5mb3JtaW5nIHVzZXIgYWJv dXQgd2hhdCdzIHdyb25nLgo+Pj4+Pgo+Pj4+PiAtIEV4cGVjdGVkIHJlc3VsdDogU2FtZSBiZWhh dmlvdXIgdG8gTkZTIGFuZCBMT0NBTEZTIHN0b3JhZ2UgcGF0aAo+Pj4+PiB2YWxpZGF0aW9uLiBW YWxpZGF0ZSBMT0NBTEZTIHBhdGggaW4gd2ViYWRtaW4gYmVmb3JlIHNlbmQgaXQgdG8gdmRzbQo+ Pj4+PiBbNV0uCj4+Pj4KPj4+PiB5b3UgbWF5IGFuZCBzaG91bGQgc2VuZCBhIHBhdGNoIDopCj4+ Pj4KPj4+Pj4KPj4+Pj4gLSBOZXdiaWUgZG91YnQ6IFdvdWxkbid0IGJlIGJldHRlciB0byB2YWxp ZGF0ZSB0aGUgYm90aCBsb2NhbCBhbmQgbmZzCj4+Pj4+IHBhdGggb24gdGhlIGJhY2tlbmQsIGFj aGlldmluZyBhbGwgdXNlciBpbnRlcmZhY2VzL0FQSXM/Cj4+Pj4KPj4+PiBCZWNhdXNlIHdlIGhh dmUgYSByaWNoIGNsaWVudCBhcHAgKGd3dCksIHdlIGNhbiBwZXJmb3JtIHRoZQo+Pj4+IHZhbGlk YXRpb24gYWxzbyBpbiB0aGUgY2xpZW50IHNpZGUgdmVyeSBlYXNpbHksCj4+Pj4gd2UgZG8gdGhh dCB0byBhdm9pZCB1bm5lY2Vzc2FyeSBjYWxscyB0byB0aGUgYmFja2VuZCBzaWRlLCBhbmQgdG8K Pj4+PiBoYXZlIGEgYmV0dGVyJiByZXNwb25zaXZlIHVpCj4+Pj4gKGNsaWVudCBzaWRlIHZhbGlk YXRpb24gaXMgcGVyZm9ybWVkIGluc3RhbnRseSAtIHdpdGhvdXQgdGhlIG5lZWQgdG8KPj4+PiB3 YWl0KS4KPj4+Pgo+Pj4+IEFueXdheSwgZXZlcnkgdmFsaWRhdGlvbiBwZXJmb3JtZWQgaW4gdGhl IGNsaWVudCBzaWRlIG5lZWRzIHRvIGJlCj4+Pj4gcGVyZm9ybWVkIGFsc28gaW4gYmFja2VuZCBz aWRlIChmb3IgYXBpLCBhbmQgb3RoZXIgcmVhc29ucyAKPj4+PiAoc2VjdXJpdHk/KSkuCj4+Pgo+ Pj4gdGhlIGNsaWVudCBzaWRlIGNhbiB2YWxpZGF0ZSB0aGUgZm9ybWF0IG9mIHRoZSBwYXRoLCBu b3QgaWYgaXQgcmVhbGx5Cj4+PiBleGlzdHMuCj4+PiBkbyB5b3UgbWVhbiBsb2NhbCBzdG9yYWdl IHZpYSB0aGUgd2l6YXJkLCBvciBqdXN0IGFkZGluZyBhbm90aGVyCj4+PiBzdG9yYWdlIGRvbWFp bj8KPj4+IHRoZSAnY29uZmlndXJlIGxvY2FsIHN0b3JhZ2UnIHdpemFyZCByZXF1aXJlcyB0aGUg aG9zdCB0byBiZSBpbgo+Pj4gbWFpbnRlbmFuY2UsIHdoaWNoIG1heSBvciBtYXkgbm90IGJlIGEg cHJvYmxlbSB0byBpbXBsZW1lbnQgdGhpcyB0eXBlCj4+PiBvZiB2YWxpZGF0aW9uLgo+Pgo+PiBJ IG1lYW4ganVzdCBhZGRpbmcgYSBMT0NBTEZTIHN0b3JhZ2UgZG9tYWluIHdpdGggYW4gaW52YWxp ZCBwYXRoLCBsaWtlCj4+IHRoaXM6Cj4+IGh0dHBzOi8vcGljYXNhd2ViLmdvb2dsZS5jb20vbGgv cGhvdG8vRldOaW91MlkxMkdaTzNBamZDSDZLN1FBdjhjczZlZGFqM2ZFY01sZUI2MCAKPj4KPj4K Pj4gQ3VycmVudGx5IHdlIGhhdmUgdGhpczoKPj4gaHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xlLmNv bS9saC9waG90by9Qb2Y2WjhvaGdRQWtSVERwRUpLRy1MUUF2OGNzNmVkYWozZkVjTWxlQjYwIAo+ Pgo+Pgo+PiBJIHNlbnQgYSBwYXRjaC4uLgo+PiBodHRwOi8vZ2Vycml0Lm92aXJ0Lm9yZy80ODU3 Cj4+IC4uLiBpbiBvcmRlciB0byBkbyB0aGlzOgo+PiBodHRwczovL3BpY2FzYXdlYi5nb29nbGUu Y29tL2xoL3Bob3RvL1BnellyWkhra3ZtLVd0RmtfVUZaTHJRQXY4Y3M2ZWRhajNmRWNNbGVCNjAg Cj4+Cj4+Cj4+IEp1c3QgbGlrZSAiTmV3IE5GUyBEb21haW4iIGN1cnJlbnRseSBkb2VzOgo+PiBo dHRwczovL3BpY2FzYXdlYi5nb29nbGUuY29tL2xoL3Bob3RvL0ZkM3pXZWdXRTBUNUMydERvX3RQ WnJRQXY4Y3M2ZWRhajNmRWNNbGVCNjAgCj4+Cj4+Cj4KPiBvayAtIHNvdW5kcyBiYXNpYyBlbm91 Z2ggb2YgYSBjaGFuZ2UuIEknZCBqdXN0IG5vdGUgaW4gdGhlIHBhdGNoIHlvdSAKPiBhcmUgcmUt dXNpbmcgc2FtZSBSRUdFWCB1c2VkIGZvciB0aGUgcHJldmlvdXMgdmFsaWRhdGlvbiBoYXBwZW5p bmcgaW4gCj4gdGhlIGJhY2tlbmQgKGFuZCBhY3R1YWxseSB0cnkgdG8gcmUtdXNlIGl0LCB0byBu b3QgbWFpbnRhaW4gYSBjaGFuZ2UgCj4gaW4gdG9vIG1hbnkgcGxhY2VzMApUaGVyZSBpcyBubyB2 YWxpZGF0aW9uIGluIGJhY2tlbmQuIEkgbWVhbiwgdGhlIGVycm9yICJFcnJvciB3aGlsZSAKZXhl Y3V0aW5nIGFjdGlvbiBBZGRMb2NhbFN0b3JhZ2VEb21haW46IFJlbW90ZSBwYXRoIGlzIGlsbGVn YWwiIGlzIApjb21pbmcgZnJvbSBWRFNNOgpodHRwczovL2dpc3QuZ2l0aHViLmNvbS8yNzYyNjU2 CkFuZCBpcyBub3QgY2F1c2VkIGJ5IHJlZ2V4LiBWRFNNIGlzIGp1c3QgY29tcGFyaW5nIHBhdGgg L3Rlc3QvIHdpdGggCm9zLnBhdGguYWJzcGF0aCgpLCAvdGVzdDoKaHR0cHM6Ly9naXN0LmdpdGh1 Yi5jb20vMjg2Nzg3NAoKLS0tLS0tLS0tLS0tLS0wMTAwMDkwNjA3MDkwNzA1MDMwODAxMDUKQ29u dGVudC1UeXBlOiB0ZXh0L2h0bWw7IGNoYXJzZXQ9SVNPLTg4NTktMQpDb250ZW50LVRyYW5zZmVy LUVuY29kaW5nOiA3Yml0Cgo8aHRtbD4KICA8aGVhZD4KICAgIDxtZXRhIGNvbnRlbnQ9InRleHQv aHRtbDsgY2hhcnNldD1JU08tODg1OS0xIgogICAgICBodHRwLWVxdWl2PSJDb250ZW50LVR5cGUi PgogIDwvaGVhZD4KICA8Ym9keSBiZ2NvbG9yPSIjRkZGRkZGIiB0ZXh0PSIjMDAwMDAwIj4KICAg IE9uIDA2LzAzLzIwMTIgMDU6MDQgQU0sIEl0YW1hciBIZWltIHdyb3RlOgogICAgPGJsb2NrcXVv dGUgY2l0ZT0ibWlkOjRGQ0IxQTk0LjgwODAyMDFAcmVkaGF0LmNvbSIgdHlwZT0iY2l0ZSI+T24K ICAgICAgMDYvMDEvMjAxMiAwODoxNiBQTSwgQW1hZG9yIFBhaGltIHdyb3RlOgogICAgICA8YnI+ CiAgICAgIDxibG9ja3F1b3RlIHR5cGU9ImNpdGUiPk9uIDA2LzAxLzIwMTIgMTI6MzkgUE0sIEl0 YW1hciBIZWltIHdyb3RlOgogICAgICAgIDxicj4KICAgICAgICA8YmxvY2txdW90ZSB0eXBlPSJj aXRlIj5PbiAwNS8yMS8yMDEyIDA5OjM3IFBNLCBHaWxhZCBDaGFwbGlrCiAgICAgICAgICB3cm90 ZToKICAgICAgICAgIDxicj4KICAgICAgICAgIDxibG9ja3F1b3RlIHR5cGU9ImNpdGUiPkhpIFBh aGltLAogICAgICAgICAgICA8YnI+CiAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgVGhhbmtz IGZvciB0aGUgaW5wdXQhCiAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgQ29tbWVudHMgaW5s aW5lLgogICAgICAgICAgICA8YnI+CiAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgVGhhbmtz LAogICAgICAgICAgICA8YnI+CiAgICAgICAgICAgIEdpbGFkLgogICAgICAgICAgICA8YnI+CiAg ICAgICAgICAgIDxicj4KICAgICAgICAgICAgLS0tLS0gT3JpZ2luYWwgTWVzc2FnZSAtLS0tLQog ICAgICAgICAgICA8YnI+CiAgICAgICAgICAgIDxibG9ja3F1b3RlIHR5cGU9ImNpdGUiPkZyb206 ICJBbWFkb3IKICAgICAgICAgICAgICBwYWhpbSI8YSBjbGFzcz0ibW96LXR4dC1saW5rLXJmYzIz OTZFIiBocmVmPSJtYWlsdG86YXBhaGltQHJlZGhhdC5jb20iPiZsdDthcGFoaW1AcmVkaGF0LmNv bSZndDs8L2E+CiAgICAgICAgICAgICAgPGJyPgogICAgICAgICAgICAgIFRvOiA8YSBjbGFzcz0i bW96LXR4dC1saW5rLWFiYnJldmlhdGVkIiBocmVmPSJtYWlsdG86ZW5naW5lLWRldmVsQG92aXJ0 Lm9yZyI+ZW5naW5lLWRldmVsQG92aXJ0Lm9yZzwvYT4KICAgICAgICAgICAgICA8YnI+CiAgICAg ICAgICAgICAgU2VudDogTW9uZGF5LCBNYXkgMjEsIDIwMTIgNTo1MjozNCBQTQogICAgICAgICAg ICAgIDxicj4KICAgICAgICAgICAgICBTdWJqZWN0OiBbRW5naW5lLWRldmVsXSBMT0NBTEZTIHBh dGggdmFsaWRhdGlvbgogICAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgICA8YnI+CiAgICAg ICAgICAgICAgPGJyPgogICAgICAgICAgICAgIEhlbGxvLAogICAgICAgICAgICAgIDxicj4KICAg ICAgICAgICAgICA8YnI+CiAgICAgICAgICAgICAgSSdtIHN0YXJ0aW5nIHRvIGtub3cgdGhlIGVu Z2luZSBjb2RlLiBJIGNob3NlIGEgbGl0dGxlCiAgICAgICAgICAgICAgdW5zdGFuZGFyZGl6ZWQK ICAgICAgICAgICAgICA8YnI+CiAgICAgICAgICAgICAgYmVoYXZpb3VyIHRvIGZvbGxvdyB0aHJv dWdoIHRoZSBkZXZlbCBwcm9jZXNzLiBJIGhhdmUgYQogICAgICAgICAgICAgIHBhdGNoIGFuZAog ICAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgICBJJ2QgbGlrZSB0byBrbm93IGlmIHlvdSBm ZWxsIHJlbGV2YW50IHRvIGNvcnJlY3QgdGhpcwogICAgICAgICAgICAgIGlzc3VlOgogICAgICAg ICAgICAgIDxicj4KICAgICAgICAgICAgICA8YnI+CiAgICAgICAgICAgICAgLSBEZXNjcmlwdGlv bjogQWRkaW5nIGEgTE9DQUwgc3RvcmFnZSBbMV0sIHdlYmFkbWluIGRvZXMKICAgICAgICAgICAg ICBub3QgdmFsaWRhdGUKICAgICAgICAgICAgICA8YnI+CiAgICAgICAgICAgICAgcGF0aCBhZ2Fp bnN0IHJlZ2V4LCBzZW5kaW5kIHRoZSBpbnZhbGlkIHBhdGggKHdpdGggZmluYWwKICAgICAgICAg ICAgICBzbGFzaCkgdG8KICAgICAgICAgICAgICA8YnI+CiAgICAgICAgICAgICAgdmRzbSBbMl0g WzNdLiBCdXQsIGFkZGluZyBhIE5GUyBzdG9yYWdlLCB0aGUgcGF0aCBpcwogICAgICAgICAgICAg IHZhbGlkYXRlZAogICAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgICBiZWZvcmUgY29udGFj dGluZyB2ZHNtIFs0XSBhdm9pZGluZyBleHRyYSB2ZHNtIHByb2Nlc3NpbmcKICAgICAgICAgICAg ICBhbmQKICAgICAgICAgICAgICA8YnI+CiAgICAgICAgICAgICAgcXVpY2tseS9jbGVhcmx5IGlu Zm9ybWluZyB1c2VyIGFib3V0IHdoYXQncyB3cm9uZy4KICAgICAgICAgICAgICA8YnI+CiAgICAg ICAgICAgICAgPGJyPgogICAgICAgICAgICAgIC0gRXhwZWN0ZWQgcmVzdWx0OiBTYW1lIGJlaGF2 aW91ciB0byBORlMgYW5kIExPQ0FMRlMKICAgICAgICAgICAgICBzdG9yYWdlIHBhdGgKICAgICAg ICAgICAgICA8YnI+CiAgICAgICAgICAgICAgdmFsaWRhdGlvbi4gVmFsaWRhdGUgTE9DQUxGUyBw YXRoIGluIHdlYmFkbWluIGJlZm9yZSBzZW5kCiAgICAgICAgICAgICAgaXQgdG8gdmRzbQogICAg ICAgICAgICAgIDxicj4KICAgICAgICAgICAgICBbNV0uCiAgICAgICAgICAgICAgPGJyPgogICAg ICAgICAgICA8L2Jsb2NrcXVvdGU+CiAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgeW91IG1h eSBhbmQgc2hvdWxkIHNlbmQgYSBwYXRjaCA6KQogICAgICAgICAgICA8YnI+CiAgICAgICAgICAg IDxicj4KICAgICAgICAgICAgPGJsb2NrcXVvdGUgdHlwZT0iY2l0ZSI+CiAgICAgICAgICAgICAg PGJyPgogICAgICAgICAgICAgIC0gTmV3YmllIGRvdWJ0OiBXb3VsZG4ndCBiZSBiZXR0ZXIgdG8g dmFsaWRhdGUgdGhlIGJvdGgKICAgICAgICAgICAgICBsb2NhbCBhbmQgbmZzCiAgICAgICAgICAg ICAgPGJyPgogICAgICAgICAgICAgIHBhdGggb24gdGhlIGJhY2tlbmQsIGFjaGlldmluZyBhbGwg dXNlciBpbnRlcmZhY2VzL0FQSXM/CiAgICAgICAgICAgICAgPGJyPgogICAgICAgICAgICA8L2Js b2NrcXVvdGU+CiAgICAgICAgICAgIDxicj4KICAgICAgICAgICAgQmVjYXVzZSB3ZSBoYXZlIGEg cmljaCBjbGllbnQgYXBwIChnd3QpLCB3ZSBjYW4gcGVyZm9ybSB0aGUKICAgICAgICAgICAgPGJy PgogICAgICAgICAgICB2YWxpZGF0aW9uIGFsc28gaW4gdGhlIGNsaWVudCBzaWRlIHZlcnkgZWFz aWx5LAogICAgICAgICAgICA8YnI+CiAgICAgICAgICAgIHdlIGRvIHRoYXQgdG8gYXZvaWQgdW5u ZWNlc3NhcnkgY2FsbHMgdG8gdGhlIGJhY2tlbmQgc2lkZSwKICAgICAgICAgICAgYW5kIHRvCiAg ICAgICAgICAgIDxicj4KICAgICAgICAgICAgaGF2ZSBhIGJldHRlciZhbXA7IHJlc3BvbnNpdmUg dWkKICAgICAgICAgICAgPGJyPgogICAgICAgICAgICAoY2xpZW50IHNpZGUgdmFsaWRhdGlvbiBp cyBwZXJmb3JtZWQgaW5zdGFudGx5IC0gd2l0aG91dCB0aGUKICAgICAgICAgICAgbmVlZCB0bwog ICAgICAgICAgICA8YnI+CiAgICAgICAgICAgIHdhaXQpLgogICAgICAgICAgICA8YnI+CiAgICAg ICAgICAgIDxicj4KICAgICAgICAgICAgQW55d2F5LCBldmVyeSB2YWxpZGF0aW9uIHBlcmZvcm1l ZCBpbiB0aGUgY2xpZW50IHNpZGUgbmVlZHMKICAgICAgICAgICAgdG8gYmUKICAgICAgICAgICAg PGJyPgogICAgICAgICAgICBwZXJmb3JtZWQgYWxzbyBpbiBiYWNrZW5kIHNpZGUgKGZvciBhcGks IGFuZCBvdGhlciByZWFzb25zCiAgICAgICAgICAgIChzZWN1cml0eT8pKS4KICAgICAgICAgICAg PGJyPgogICAgICAgICAgPC9ibG9ja3F1b3RlPgogICAgICAgICAgPGJyPgogICAgICAgICAgdGhl IGNsaWVudCBzaWRlIGNhbiB2YWxpZGF0ZSB0aGUgZm9ybWF0IG9mIHRoZSBwYXRoLCBub3QgaWYg aXQKICAgICAgICAgIHJlYWxseQogICAgICAgICAgPGJyPgogICAgICAgICAgZXhpc3RzLgogICAg ICAgICAgPGJyPgogICAgICAgICAgZG8geW91IG1lYW4gbG9jYWwgc3RvcmFnZSB2aWEgdGhlIHdp emFyZCwgb3IganVzdCBhZGRpbmcKICAgICAgICAgIGFub3RoZXIKICAgICAgICAgIDxicj4KICAg ICAgICAgIHN0b3JhZ2UgZG9tYWluPwogICAgICAgICAgPGJyPgogICAgICAgICAgdGhlICdjb25m aWd1cmUgbG9jYWwgc3RvcmFnZScgd2l6YXJkIHJlcXVpcmVzIHRoZSBob3N0IHRvIGJlCiAgICAg ICAgICBpbgogICAgICAgICAgPGJyPgogICAgICAgICAgbWFpbnRlbmFuY2UsIHdoaWNoIG1heSBv ciBtYXkgbm90IGJlIGEgcHJvYmxlbSB0byBpbXBsZW1lbnQKICAgICAgICAgIHRoaXMgdHlwZQog ICAgICAgICAgPGJyPgogICAgICAgICAgb2YgdmFsaWRhdGlvbi4KICAgICAgICAgIDxicj4KICAg ICAgICA8L2Jsb2NrcXVvdGU+CiAgICAgICAgPGJyPgogICAgICAgIEkgbWVhbiBqdXN0IGFkZGlu ZyBhIExPQ0FMRlMgc3RvcmFnZSBkb21haW4gd2l0aCBhbiBpbnZhbGlkCiAgICAgICAgcGF0aCwg bGlrZQogICAgICAgIDxicj4KICAgICAgICB0aGlzOgogICAgICAgIDxicj4KPGEgY2xhc3M9Im1v ei10eHQtbGluay1mcmVldGV4dCIgaHJlZj0iaHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xlLmNvbS9s aC9waG90by9GV05pb3UyWTEyR1pPM0FqZkNINks3UUF2OGNzNmVkYWozZkVjTWxlQjYwIj5odHRw czovL3BpY2FzYXdlYi5nb29nbGUuY29tL2xoL3Bob3RvL0ZXTmlvdTJZMTJHWk8zQWpmQ0g2SzdR QXY4Y3M2ZWRhajNmRWNNbGVCNjA8L2E+CiAgICAgICAgPGJyPgogICAgICAgIDxicj4KICAgICAg ICBDdXJyZW50bHkgd2UgaGF2ZSB0aGlzOgogICAgICAgIDxicj4KPGEgY2xhc3M9Im1vei10eHQt bGluay1mcmVldGV4dCIgaHJlZj0iaHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xlLmNvbS9saC9waG90 by9Qb2Y2WjhvaGdRQWtSVERwRUpLRy1MUUF2OGNzNmVkYWozZkVjTWxlQjYwIj5odHRwczovL3Bp Y2FzYXdlYi5nb29nbGUuY29tL2xoL3Bob3RvL1BvZjZaOG9oZ1FBa1JURHBFSktHLUxRQXY4Y3M2 ZWRhajNmRWNNbGVCNjA8L2E+CiAgICAgICAgPGJyPgogICAgICAgIDxicj4KICAgICAgICBJIHNl bnQgYSBwYXRjaC4uLgogICAgICAgIDxicj4KICAgICAgICA8YSBjbGFzcz0ibW96LXR4dC1saW5r LWZyZWV0ZXh0IiBocmVmPSJodHRwOi8vZ2Vycml0Lm92aXJ0Lm9yZy80ODU3Ij5odHRwOi8vZ2Vy cml0Lm92aXJ0Lm9yZy80ODU3PC9hPgogICAgICAgIDxicj4KICAgICAgICAuLi4gaW4gb3JkZXIg dG8gZG8gdGhpczoKICAgICAgICA8YnI+CjxhIGNsYXNzPSJtb3otdHh0LWxpbmstZnJlZXRleHQi IGhyZWY9Imh0dHBzOi8vcGljYXNhd2ViLmdvb2dsZS5jb20vbGgvcGhvdG8vUGd6WXJaSGtrdm0t V3RGa19VRlpMclFBdjhjczZlZGFqM2ZFY01sZUI2MCI+aHR0cHM6Ly9waWNhc2F3ZWIuZ29vZ2xl LmNvbS9saC9waG90by9QZ3pZclpIa2t2bS1XdEZrX1VGWkxyUUF2OGNzNmVkYWozZkVjTWxlQjYw PC9hPgogICAgICAgIDxicj4KICAgICAgICA8YnI+CiAgICAgICAgSnVzdCBsaWtlICJOZXcgTkZT IERvbWFpbiIgY3VycmVudGx5IGRvZXM6CiAgICAgICAgPGJyPgo8YSBjbGFzcz0ibW96LXR4dC1s aW5rLWZyZWV0ZXh0IiBocmVmPSJodHRwczovL3BpY2FzYXdlYi5nb29nbGUuY29tL2xoL3Bob3Rv L0ZkM3pXZWdXRTBUNUMydERvX3RQWnJRQXY4Y3M2ZWRhajNmRWNNbGVCNjAiPmh0dHBzOi8vcGlj YXNhd2ViLmdvb2dsZS5jb20vbGgvcGhvdG8vRmQzeldlZ1dFMFQ1QzJ0RG9fdFBaclFBdjhjczZl ZGFqM2ZFY01sZUI2MDwvYT4KICAgICAgICA8YnI+CiAgICAgICAgPGJyPgogICAgICA8L2Jsb2Nr cXVvdGU+CiAgICAgIDxicj4KICAgICAgb2sgLSBzb3VuZHMgYmFzaWMgZW5vdWdoIG9mIGEgY2hh bmdlLiBJJ2QganVzdCBub3RlIGluIHRoZSBwYXRjaAogICAgICB5b3UgYXJlIHJlLXVzaW5nIHNh bWUgUkVHRVggdXNlZCBmb3IgdGhlIHByZXZpb3VzIHZhbGlkYXRpb24KICAgICAgaGFwcGVuaW5n IGluIHRoZSBiYWNrZW5kIChhbmQgYWN0dWFsbHkgdHJ5IHRvIHJlLXVzZSBpdCwgdG8gbm90CiAg ICAgIG1haW50YWluIGEgY2hhbmdlIGluIHRvbyBtYW55IHBsYWNlczAKICAgICAgPGJyPgogICAg PC9ibG9ja3F1b3RlPgogICAgVGhlcmUgaXMgbm8gdmFsaWRhdGlvbiBpbiBiYWNrZW5kLiBJIG1l YW4sIHRoZSBlcnJvciAiRXJyb3Igd2hpbGUKICAgIGV4ZWN1dGluZyBhY3Rpb24gQWRkTG9jYWxT dG9yYWdlRG9tYWluOiBSZW1vdGUgcGF0aCBpcyBpbGxlZ2FsIiBpcwogICAgY29taW5nIGZyb20g VkRTTTo8YnI+CiAgICA8bWV0YSBodHRwLWVxdWl2PSJjb250ZW50LXR5cGUiIGNvbnRlbnQ9InRl eHQvaHRtbDsKICAgICAgY2hhcnNldD1JU08tODg1OS0xIj4KICAgIDxhIGhyZWY9Imh0dHBzOi8v Z2lzdC5naXRodWIuY29tLzI3NjI2NTYiPmh0dHBzOi8vZ2lzdC5naXRodWIuY29tLzI3NjI2NTY8 L2E+PGJyPgogICAgQW5kIGlzIG5vdCBjYXVzZWQgYnkgcmVnZXguIFZEU00gaXMganVzdCBjb21w YXJpbmcgcGF0aCAvdGVzdC8gd2l0aAogICAgb3MucGF0aC5hYnNwYXRoKCksIC90ZXN0Ojxicj4K ICAgIDxtZXRhIGh0dHAtZXF1aXY9ImNvbnRlbnQtdHlwZSIgY29udGVudD0idGV4dC9odG1sOwog ICAgICBjaGFyc2V0PUlTTy04ODU5LTEiPgogICAgPGEgaHJlZj0iaHR0cHM6Ly9naXN0LmdpdGh1 Yi5jb20vMjg2Nzg3NCI+aHR0cHM6Ly9naXN0LmdpdGh1Yi5jb20vMjg2Nzg3NDwvYT48YnI+CiAg PC9ib2R5Pgo8L2h0bWw+CgotLS0tLS0tLS0tLS0tLTAxMDAwOTA2MDcwOTA3MDUwMzA4MDEwNS0t Cg== --===============8992161935189450156==-- From iheim at redhat.com Mon Jun 4 08:08:43 2012 Content-Type: multipart/mixed; boundary="===============3518529096520421215==" MIME-Version: 1.0 From: Itamar Heim To: devel at ovirt.org Subject: Re: [Engine-devel] LOCALFS path validation Date: Mon, 04 Jun 2012 15:08:40 +0300 Message-ID: <4FCCA548.3000609@redhat.com> In-Reply-To: 4FCCA0FA.9020002@redhat.com --===============3518529096520421215== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On 06/04/2012 02:50 PM, Amador Pahim wrote: > 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" >>>>>> 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/FWNiou2Y12GZO3AjfCH6K7QAv8cs6edaj= 3fEcMleB60 >>> >>> >>> Currently we have this: >>> https://picasaweb.google.com/lh/photo/Pof6Z8ohgQAkRTDpEJKG-LQAv8cs6edaj= 3fEcMleB60 >>> >>> >>> I sent a patch... >>> http://gerrit.ovirt.org/4857 >>> ... in order to do this: >>> https://picasaweb.google.com/lh/photo/PgzYrZHkkvm-WtFk_UFZLrQAv8cs6edaj= 3fEcMleB60 >>> >>> >>> Just like "New NFS Domain" currently does: >>> https://picasaweb.google.com/lh/photo/Fd3zWegWE0T5C2tDo_tPZrQAv8cs6edaj= 3fEcMleB60 >>> >>> >> >> 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 sounds like a valid validation to add then --===============3518529096520421215==--