Skip to content

Fixed incorrect error message on invalid template type download #4138

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.

Sign up for GitHub

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

Merged
yadvr merged 7 commits into from
Jul 15, 2020
Merged

Fixed incorrect error message on invalid template type download #4138

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9775dd5
Added a check for the original error message
Spaceman1984 Jun 10, 2020
dabb83e
Added some cleanup
Spaceman1984 Jun 10, 2020
4db5f19
Changed empty string check
Spaceman1984 Jun 10, 2020
c6ccb52
Initialised errorString to null and added exception when format is in…
Spaceman1984 Jun 23, 2020
fdfe7e8
Reverting this change due to unintended side effects
Spaceman1984 Jun 24, 2020
977335e
Formatting
Spaceman1984 Jul 3, 2020
b77a929
Removed executable flag from file
Spaceman1984 Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions core/src/main/java/com/cloud/storage/template/HttpTemplateDownloader.java
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.net.URISyntaxException;
import java.util.Date;

import com.cloud.utils.exception.CloudRuntimeException;
import org.apache.cloudstack.utils.imagestore.ImageStoreUtil;
import org.apache.commons.httpclient.Credentials;
import org.apache.commons.httpclient.Header;
Expand Down Expand Up @@ -63,7 +64,7 @@ public class HttpTemplateDownloader extends ManagedContextRunnable implements Te
private String downloadUrl;
private String toFile;
public TemplateDownloader.Status status;
public String errorString = " ";
private String errorString = null;
private long remoteSize = 0;
public long downloadTime = 0;
public long totalBytes;
Expand Down Expand Up @@ -218,7 +219,10 @@ public long download(boolean resume, DownloadCompleteCallback callback) {
errorString = hte.getMessage();
Comment thread
Spaceman1984 marked this conversation as resolved.
} catch (IOException ioe) {
status = TemplateDownloader.Status.UNRECOVERABLE_ERROR; //probably a file write error?
errorString = ioe.getMessage();
// Let's not overwrite the original error message.
if (errorString == null) {
errorString = ioe.getMessage();
}
} finally {
if (status == Status.UNRECOVERABLE_ERROR && file.exists() && !file.isDirectory()) {
file.delete();
Expand All @@ -243,7 +247,6 @@ private boolean copyBytes(File file, InputStream in, RandomAccessFile out) throw
offset = writeBlock(bytes, out, block, offset);
if (!verifyFormat.isVerifiedFormat() && (offset >= 1048576 || offset >= remoteSize)) { //let's check format after we get 1MB or full file
verifyFormat.invoke();
if (verifyFormat.isInvalid()) return true;
Copy link
Copy Markdown
Member

@yadvr yadvr Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Spaceman1984 cc @nvazquez why are we not validating format now?

Copy link
Copy Markdown
Contributor Author

@Spaceman1984 Spaceman1984 Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhtyd If validation fails, an exception is thrown, so there is no setting of an invalid state that has to be checked. Validation happens inside verifyFormat.invoke();

Copy link
Copy Markdown
Contributor

@nvazquez nvazquez Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short, prior to these changes the invoke() method only set the boolean variable retrieved in case of valid/invalid. After these changes, if it is invalid invoke() throws an exception, so no need to check isInvalid() in this line

}
} else {
done = true;
Expand Down Expand Up @@ -443,7 +446,7 @@ public boolean isResume() {

@Override
public String getDownloadError() {
return errorString;
return errorString == null ? " " : errorString;
}

@Override
Expand Down Expand Up @@ -495,7 +498,6 @@ public ResourceType getResourceType() {
}

private class VerifyFormat {
private boolean invalidFormat;
private File file;
private boolean verifiedFormat;

Expand All @@ -504,10 +506,6 @@ public VerifyFormat(File file) {
this.verifiedFormat = false;
}

boolean isInvalid() {
return invalidFormat;
}

public boolean isVerifiedFormat() {
return verifiedFormat;
}
Expand All @@ -529,11 +527,10 @@ public VerifyFormat invoke() {
}
status = Status.UNRECOVERABLE_ERROR;
errorString = "Template content is unsupported, or mismatch between selected format and template content. Found : " + unsupportedFormat;
invalidFormat = true;
throw new CloudRuntimeException(errorString);
} else {
s_logger.debug("Verified format of downloading file " + file.getAbsolutePath() + " is supported");
verifiedFormat = true;
invalidFormat = false;
}
return this;
}
Expand Down