Re: [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.