Re: [PATCH 2/3] msvcp110: Add tr2_sys__Symlink implementation and test.

[Piotr Caban]

On 01/11/16 14:56, YongHao Hu wrote:
> - delete_flag = 1;
> - for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) {
> -
>    errno = 0xdeadbeef;
> -
>    SetLastError(0xdeadbeef);
> -
>    ret = p_tr2_sys__Symlink(tests[i].existing_path, tests[i].new_path);
> -
>    if(ret==ERROR_PRIVILEGE_NOT_HELD || ret==ERROR_INVALID_FUNCTION) {
> -
>        delete_flag = 0;
> -
>        tests[i].last_error = ret;
> -
>        win_skip("Privilege not held or symbolic link not supported, skipping symbolic link tests.\n");

I think it would be nicer if the tests are really skipped in this case. 
Please detect such case and return from the function instead of adding 
workarounds in the code for it.

Thanks,
Piotr

[PATCH 2/3] msvcp110: Add tr2_sys__Symlink implementation and test.

[YongHao Hu]

Hi, Piotr.
Sorry for the late reply.

On 16/1/12 下午9:28, Piotr Caban wrote:
> On 01/11/16 14:56, YongHao Hu wrote:
> > - delete_flag = 1;
> > - for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) {
> > -
> >    errno = 0xdeadbeef;
> > -
> >    SetLastError(0xdeadbeef);
> > -
> >    ret = p_tr2_sys__Symlink(tests[i].existing_path, 
> >
> > tests[i].new_path);
> > -
> >    if(ret==ERROR_PRIVILEGE_NOT_HELD || 
> >
> > ret==ERROR_INVALID_FUNCTION) {
> > -
> >        delete_flag = 0;
> > -
> >        tests[i].last_error = ret;
> > -
> >        win_skip("Privilege not held or symbolic link not 
> >
> > supported, skipping symbolic link tests.\n");
>
> I think it would be nicer if the tests are really skipped in this 
> case. Please detect such case and return from the function instead of 
> adding workarounds in the code for it.

Could you explain a bit more details？
Thanks.

[PATCH 2/3] msvcp110: Add tr2_sys__Symlink implementation and test.

[Piotr Caban]

Hi,

On 02/05/16 10:30, YongHao Hu wrote:
> On 16/1/12 下午9:28, Piotr Caban wrote:
> > On 01/11/16 14:56, YongHao Hu wrote:
> > > - delete_flag = 1;
> > > - for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) {
> > > -
> > >    errno = 0xdeadbeef;
> > > -
> > >    SetLastError(0xdeadbeef);
> > > -
> > >    ret = p_tr2_sys__Symlink(tests[i].existing_path,
> > >
> > > tests[i].new_path);
> > > -
> > >    if(ret==ERROR_PRIVILEGE_NOT_HELD ||
> > >
> > > ret==ERROR_INVALID_FUNCTION) {
> > > -
> > >        delete_flag = 0;
> > > -
> > >        tests[i].last_error = ret;
> > > -
> > >        win_skip("Privilege not held or symbolic link not
> > >
> > > supported, skipping symbolic link tests.\n");
> >
> > I think it would be nicer if the tests are really skipped in this
> > case. Please detect such case and return from the function instead of
> > adding workarounds in the code for it.
>
> Could you explain a bit more details？

You're trying to create the symlinks in loop. Because of this if 
symbolic links are not supported you're printing this information many 
times. I think you should try to create one symlink and if it doesn't 
work just skip all of the tests.

Thanks,
Piotr

[PATCH 2/3] msvcp110: Add tr2_sys__Symlink implementation and test.

[YongHao Hu]

Hi,

On 16/2/8 下午7:11, Piotr Caban wrote:
> Hi,
>
> On 02/05/16 10:30, YongHao Hu wrote:
> > On 16/1/12 下午9:28, Piotr Caban wrote:
> > > On 01/11/16 14:56, YongHao Hu wrote:
> > > > - delete_flag = 1;
> > > > - for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) {
> > > > -
> > > >    errno = 0xdeadbeef;
> > > > -
> > > >    SetLastError(0xdeadbeef);
> > > > -
> > > >    ret = p_tr2_sys__Symlink(tests[i].existing_path,
> > > >
> > > > tests[i].new_path);
> > > > -
> > > >    if(ret==ERROR_PRIVILEGE_NOT_HELD ||
> > > >
> > > > ret==ERROR_INVALID_FUNCTION) {
> > > > -
> > > >        delete_flag = 0;
> > > > -
> > > >        tests[i].last_error = ret;
> > > > -
> > > >        win_skip("Privilege not held or symbolic link not
> > > >
> > > > supported, skipping symbolic link tests.\n");
> > >
> > > I think it would be nicer if the tests are really skipped in this
> > > case. Please detect such case and return from the function instead of
> > > adding workarounds in the code for it.
> >
> > Could you explain a bit more details？
>
> You're trying to create the symlinks in loop. Because of this if 
> symbolic links are not supported you're printing this information many 
> times. I think you should try to create one symlink and if it doesn't 
> work just skip all of the tests.

Do you think adding a break in the loop after detecting such case is 
good?For example:

delete_flag = 1;
         for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) {
             errno = 0xdeadbeef;
             SetLastError(0xdeadbeef);
             ret = p_tr2_sys__Symlink(tests[i].existing_path,
    tests[i].new_path);
             if(ret==ERROR_PRIVILEGE_NOT_HELD ||
    ret==ERROR_INVALID_FUNCTION) {
                 delete_flag = 0;
                 tests[i].last_error = ret;
                 win_skip("Privilege not held or symbolic link not
    supported, skipping symbolic link tests.\n");
    +              break;
             }
       }

Thank you.