Juan Hernandez has posted comments on this change. Change subject: restapi: Add ability to extract rsdl.xml from jar ......................................................................
Patch Set 1: (8 comments) http://gerrit.ovirt.org/#/c/24385/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/rsdl/RsdlIOManager.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/rsdl/RsdlIOManager.java: Line 10: public class RsdlIOManager { Line 11: Line 12: private static final int DEFAULT_BUFFER_SIZE = 1024 * 4; Line 13: public static final String RSDL_FILE_NAME = "/rsdl.xml"; Line 14: public static final String GLUSTER_RSDL_FILE_NAME = "/rsdl_gluster.xml"; These ^aren't file names, rather resource names. I suggest to name them _RESOURCE_NAME instead of _FILE_NAME. Line 15: Line 16: public static void main(String[] args) throws IOException { Line 17: System.out.println("Copying rsdl.xml, rsdl_gluster.xml to: " + args[0]); Line 18: copyRsdl(args[0]); Line 13: public static final String RSDL_FILE_NAME = "/rsdl.xml"; Line 14: public static final String GLUSTER_RSDL_FILE_NAME = "/rsdl_gluster.xml"; Line 15: Line 16: public static void main(String[] args) throws IOException { Line 17: System.out.println("Copying rsdl.xml, rsdl_gluster.xml to: " + args[0]); We will probably run this from maven, and will use it to write to a "target/generated-metadata" directory, which won't initially exist, so please create the directory here if it doesn't exist: File outputDir = new File(args[0]); if (!outputDir.exists()) { outputDir.mkdirs(); } Line 18: copyRsdl(args[0]); Line 19: } Line 20: Line 21: public static void copyRsdl(String outputDirectory) throws IOException { Line 18: copyRsdl(args[0]); Line 19: } Line 20: Line 21: public static void copyRsdl(String outputDirectory) throws IOException { Line 22: copy(loadAsStream(RSDL_FILE_NAME), new FileWriter(new File(outputDirectory + RSDL_FILE_NAME))); It isn't good to create the File object concatenating the directory and the file name, because the operating system may use \ instead of / to separate paths. Use the constructor of File that takes two arguments: File rsdlOutputFile = new File(outputDirectory, RSDL_FILE_NAME); Line 23: copy(loadAsStream(GLUSTER_RSDL_FILE_NAME), new FileWriter(new File(outputDirectory + GLUSTER_RSDL_FILE_NAME))); Line 24: } Line 25: Line 26: public static InputStream loadAsStream(String fileName) { Line 19: } Line 20: Line 21: public static void copyRsdl(String outputDirectory) throws IOException { Line 22: copy(loadAsStream(RSDL_FILE_NAME), new FileWriter(new File(outputDirectory + RSDL_FILE_NAME))); Line 23: copy(loadAsStream(GLUSTER_RSDL_FILE_NAME), new FileWriter(new File(outputDirectory + GLUSTER_RSDL_FILE_NAME))); This never closes the streams. Try to do something like this: InputStream in = null; OutputStream out = null; try { in = ...; out = ...; } finally { if (in != null) { in.close(); } if (out != null) { out.close(); } } If you don't do this the file descriptors will be leaked. Not a big issue when running the code generator, but worth fixing anyhow. Line 24: } Line 25: Line 26: public static InputStream loadAsStream(String fileName) { Line 27: return RsdlIOManager.class.getResourceAsStream(fileName); Line 22: copy(loadAsStream(RSDL_FILE_NAME), new FileWriter(new File(outputDirectory + RSDL_FILE_NAME))); Line 23: copy(loadAsStream(GLUSTER_RSDL_FILE_NAME), new FileWriter(new File(outputDirectory + GLUSTER_RSDL_FILE_NAME))); Line 24: } Line 25: Line 26: public static InputStream loadAsStream(String fileName) { I would use resourceName instead of fileName. Line 27: return RsdlIOManager.class.getResourceAsStream(fileName); Line 28: } Line 29: Line 30: public static void copy(InputStream input, Writer output) throws IOException { Line 26: public static InputStream loadAsStream(String fileName) { Line 27: return RsdlIOManager.class.getResourceAsStream(fileName); Line 28: } Line 29: Line 30: public static void copy(InputStream input, Writer output) throws IOException { If the first parameter is a input stream the second should be an output stream, not a writer, otherwise you will be generating the output with an unknown encoding. Line 31: InputStreamReader in = new InputStreamReader(input); Line 32: char[] buffer = new char[DEFAULT_BUFFER_SIZE]; Line 33: int n = 0; Line 34: while (-1 != (n = in.read(buffer))) { Line 28: } Line 29: Line 30: public static void copy(InputStream input, Writer output) throws IOException { Line 31: InputStreamReader in = new InputStreamReader(input); Line 32: char[] buffer = new char[DEFAULT_BUFFER_SIZE]; Use byte[], not char[], together with stream instead of writer. Line 33: int n = 0; Line 34: while (-1 != (n = in.read(buffer))) { Line 35: output.write(buffer, 0, n); Line 36: } http://gerrit.ovirt.org/#/c/24385/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/rsdl/RsdlManager.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/rsdl/RsdlManager.java: Line 54: String fileName = Line 55: applicationMode == ApplicationMode.AllModes ? RsdlIOManager.RSDL_FILE_NAME Line 56: : RsdlIOManager.GLUSTER_RSDL_FILE_NAME; Line 57: InputStream rsdlAsStrem = RsdlIOManager.loadAsStream(fileName); Line 58: return JAXB.unmarshal(rsdlAsStrem, RSDL.class); The input stream is never closed here. Line 59: } Line 60: Line 61: private static void serializeRsdl(RSDL rsdl, String rsdlLocation) { Line 62: JAXB.marshal(rsdl, new File(rsdlLocation)); -- To view, visit http://gerrit.ovirt.org/24385 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I11834dac081e17c702c614ae923575a3debba6eb Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ori Liel <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
