Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SETUPAPI] Avoid stray temp files in failure cases of do_file_copyW #408

Closed
wants to merge 1 commit into from

Conversation

@ThFabba
Copy link
Member

commented Mar 2, 2018

JIRA issue: CORE-12616

@ThFabba ThFabba requested review from HBelusca and yagoulas Mar 2, 2018

ERR("LZOpenFileW(2) error %d %s\n", (int)hTemp, debugstr_w(TempFile));

/* Close the source handle */
LZClose(hSource);

/* Restore error condition triggered by LZOpenFileW */

This comment has been minimized.

Copy link
@ThFabba

ThFabba Mar 3, 2018

Author Member

I'm not sure I understand why this was removed. I'm assuming we'd still want the last error to be sensible?

This comment has been minimized.

Copy link
@learn-more

learn-more Mar 12, 2018

Member

In that case we should move it after DeleteFileW

This comment has been minimized.

Copy link
@gedmurphy

gedmurphy May 17, 2018

Member

I'd argue this should be put back, and also added to the GetTempFileNameW failure handler and the 'if (lRes < 0)' handler at line 1121
Do we have any tests that confirm?

This comment has been minimized.

Copy link
@ColinFinck

ColinFinck Jun 27, 2018

Member

@gedmurphy Your comment sounds like a blocker for merging this PR to master. If that's true, please signal this by clicking Review changes -> Request changes. Otherwise, according to the new PR guidelines, this PR fulfills all requirements and would be merged at the next occasion.

Of course, @ThFabba is also free to simply address the comment and fix the code accordingly.

@JoachimHenze

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

Just wanted to mention: I applied this manually to 0.4.8RC already and it works well for me from testers pov. Would love to see it in master as well.

@sanchaez sanchaez added the bugfix label Apr 7, 2018

@gedmurphy gedmurphy self-requested a review Jun 27, 2018

@gedmurphy
Copy link
Member

left a comment

Please see my comment on May 17

@binarymaster

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2018

@carlo-bramini please associate this email with your GitHub account, so the commit in this PR will be associated with you:

Click to show e-mail

carlo.bramix@libero.it

@JoachimHenze JoachimHenze self-requested a review Oct 31, 2018

@JoachimHenze
Copy link
Contributor

left a comment

I am no fan of blocking PR408 endlessly. Let's merge it now to allow CORE-12616 to turn into resolved state.
We have known regression since more than two years in master now without this PR.
I committed the contents of this PR into 0.4.8rls, 0.4.9rls and 0.4.10rls already. It did not break anything and it is definitely a big improvement over current regressed master state.
If it's not perfect yet, then let's do further development in Sergeys ticket CORE-12542 later.
How long do we want to leave master broken? After all, I will nevertheless commit this repeatedly in every upcoming release anyway, so we could end this recurrent work by committing now.

@HeisSpiter HeisSpiter self-assigned this Nov 2, 2018

@HeisSpiter

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Manually applied along with a patch.

@HeisSpiter HeisSpiter closed this Nov 2, 2018

@gedmurphy

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Cool, thanks Pierre, they're the changes I was hoping for 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.