GNOME Bugzilla – Bug 700050
Convert POSIX I/O to GIO and GFile in EasyTAG.
Last modified: 2021-05-26 09:51:37 UTC
Currently EasyTAG uses POSIX IO i.e. functions fread, fwrite, fopen, fclose, close, write, read, open. POSIX IO could be replaced with much better and high level GFile from GIO. This will simplify file handling, name of file handling encoding, could also help in making Asynchronous I/O and with this EasyTAG could be used over NFS and CIFS/Samba shares.
Created attachment 243762 [details] [review] For log.c all changes done to transfer POSIX IO to GFile in GIO Please review and let me know if this is a correct way.
Review of attachment 243762 [details] [review]: Basically the write approach, with a few changes needed. ::: src/log.c @@ +334,3 @@ gchar *data = g_strdup_printf("%s %s\n",time,string); + if (g_output_stream_write (G_OUTPUT_STREAM (file_ostream), data, 1, + NULL, &error) != 1) This only writes a single byte of the buffer to the log. Remember that the previous fwrite call wrote 1 item of the buffer to the stream, where the number of bytes in the item was the length of the buffer. Also, check the indentation on the second line. @@ +342,2 @@ g_free (string); + g_error_free (error); Rather than freeing the error, you should report it in the g_critical() above and then free it. @@ +342,3 @@ g_free (string); + g_error_free (error); + if (!g_output_stream_close (G_OUTPUT_STREAM (file_ostream), NULL, &error)) You need to break this line so that it is under 80 characters long. @@ +345,3 @@ + { + g_critical ("Error closing output stream of file '%s' ('%s')", + file_path, error->message); Need to indent by an extra space here. @@ +351,1 @@ return; You need to unref "file_ostream" before returning. @@ +358,3 @@ + { + g_critical ("Error closing output stream of file '%s' ('%s')", + file_path, error->message); Need to indent by an extra space here. @@ +365,3 @@ + { + g_critical ("Error opening output stream of file '%s' ('%s')", + file_path, error->message); Need to indent by an extra space here. @@ +369,3 @@ } + g_object_unref (file); You also need to unref "file_ostream" before returning.
Created attachment 243764 [details] [review] Converted POSIX IO to GIO for log.c
Review of attachment 243764 [details] [review]: ::: src/log.c @@ +334,3 @@ gchar *data = g_strdup_printf("%s %s\n",time,string); + if (g_output_stream_write (G_OUTPUT_STREAM (file_ostream), data, strlen (data), + NULL, &error) != strlen (data)) Rather than checking the length of the string twice, use a GString (and g_string_append to construct the string and to avoid using format specifiers). Then, use the "len" member of GString as the length. @@ +373,3 @@ } + g_object_unref (file); You still do not unref the output stream before returning, specifically in the no-error case.
Created attachment 243771 [details] [review] Done for log.c
Created attachment 243772 [details] [review] Done for log.c
Review of attachment 243772 [details] [review]: ::: src/log.c @@ +256,3 @@ + GFileOutputStream *file_ostream; + GError *error = NULL; + GString *data; You should move this definition to the block where it is used to reduce the scope as much as possible. @@ +335,3 @@ + data = g_string_new (time); + data = g_string_append (data, string); + data = g_string_append_c (data, '\n'); You also need to append a space between "data" and "string". @@ +360,1 @@ g_free(time); You should move this and the same line inside the above if to be just after appending to the GString.
Created attachment 243783 [details] [review] Did the above things, for log.c`
Review of attachment 243783 [details] [review]: I was about to commit this and then realised that there was lots of junk in the log file. Please double-check how you write the buffer to the file.
Created attachment 243792 [details] [review] Double Checked and corrected I accidentally wrote "data" instead of "data->str" a big silly mistake by me.
Review of attachment 243792 [details] [review]: ::: src/log.c @@ +327,2 @@ else + file_ostream = g_file_append_to (file, G_FILE_CREATE_NONE, NULL, Be aware that this does not set the stream to the end of the file, so the log file is always overwritten. Luckily, this means that you can simplify the code to always append, creating the file if it does not exist. You should then seek to the end of the file before writing new log data.
Created attachment 243799 [details] [review] Only appending to file I don't think seeking to the end is required. Because, g_file_append_to will either create a new file for writing or will automatically adjust the seek to the end position of file to append. This is the behaviour we want. Moreover, the patch is working for me giving the right output. Review it please and tell me if there is some problem.
Comment on attachment 243792 [details] [review] Double Checked and corrected I realised that the original code intentionally recreated the log file on startup, so I pushed a modified version of this patch to master as commit 31beeb7903700e6e28f2c3c9c8e8f6302a3dab0b.
Created attachment 243830 [details] [review] Converted POSIX IO to GIO Here are few things: 1. originally in Picture_Save_File_Data, "mode" argument of "open" was 0666. This is default for file and means read and write permission to everyone (As far as I know) so I passed "G_FILE_CREATE_NONE" as "GFileCreateArguments" to "g_file_replace". 2. "open" was passed flags O_WRONLY|O_CREAT|O_TRUNC. This means, create a file if it doesn't exists, if it exists then truncate the file. This behaviour is achieved by "g_file_replace". Hence, I used "g_file_replace". Thank You.
Review of attachment 243830 [details] [review]: ::: src/picture.c @@ +1179,3 @@ + &error)) + { + Log_Print (LOG_ERROR, _("Image file not closed: %s"), There is little point in reporting an error when the file is closed, so just pass NULL instead of &error. @@ +1192,3 @@ + &error)) + { + Log_Print (LOG_ERROR, _("Image file not closed: %s"), Again, not much point in reporting the error on close, so just pass NULL instead of &error. @@ +1242,3 @@ + NULL, &error)) + { + Log_Print (LOG_ERROR, _("Image file cannot be closed: %s"), Same comment about reporting an error on close. @@ +1255,3 @@ + NULL, &error)) + { + Log_Print (LOG_ERROR, _("Image file cannot be closed: %s"), Same comment about reporting an error on close.
Review of attachment 243830 [details] [review]: ::: src/picture.c @@ +1192,3 @@ + &error)) + { + Log_Print (LOG_ERROR, _("Image file not closed: %s"), Ah, of course this is fine, as there was no previous error, so ignore my earlier comment. @@ +1255,3 @@ + NULL, &error)) + { + Log_Print (LOG_ERROR, _("Image file cannot be closed: %s"), This is also fine, as there was no previous error, so ignore my earlier comment.
Created attachment 243834 [details] [review] Did the changes
Review of attachment 243834 [details] [review]: Actually, on reading the documentation for g_io_stream_close() a bit more, the stream is automatically closed when the last reference is dropped, so I think that you should drop all the explicit g_io_stream_close() calls and just unref the iostream instead.
Created attachment 243836 [details] [review] Used g_object_unref instead of g_stream_close I don't think everywhere g_object_unref could be used instead of g_input_stream_close and g_output_stream_close. Because, there is no way of getting error with g_object_unref, when stream is closed. To get the error while closing the stream g_*_stream_close should be used. Here, I removed g_*_stream_close, where there is no error reporting required i.e. sending error as NULL to them.
Review of attachment 243836 [details] [review]: ::: src/picture.c @@ +1186,3 @@ + &error)) + { + Log_Print (LOG_ERROR, _("Image file not closed: %s"), There is almost no point returning an error here, as the image was already loaded by this point. At worst, a file descriptor will be leaked, which is not a problem that warrants an error in the EasyTAG log. At most a g_warning() should be used, but a g_message() would be better. @@ +1241,3 @@ + NULL, &error)) + { + Log_Print (LOG_ERROR, _("Image file cannot be closed: %s"), In this case, because a failing close means that all the data might not have been written to storage, you should return FALSE. As no calling function checks the return value of this one, you should likely change this function to return a GError, and propagate the error from g_output_stream_close() to any calling function. You can follow this approach in other instances where IO fails.
(In reply to comment #19) > I don't think everywhere g_object_unref could be used instead of > g_input_stream_close and g_output_stream_close. Because, there is no way of > getting error with g_object_unref, when stream is closed. To get the error > while closing the stream g_*_stream_close should be used. True, that is a good point. Clarifying my earlier comment, when loading a file, there is little point in reporting an error to the user, but it is still fine to log it with g_message(). When saving, data could be lost if the file is not successfully closed, so it is best to pass the GError returned from g_*_close() to the calling function, which can then decide what to do (pop up a dialogue, retry the operation or something else). This gives the decision on how to inform the user to the calling (UI) code, which has the extra context to be able to give a good error message.
Created attachment 243847 [details] [review] Changed Picture_Save_File_Data function to accept GError** as argument Here I have done two things more: 1. Added g_message and passed NULL to the corresponding g_stream_close function. 2. Removed all the Log_Print function from Picture_Set_File_Data. Whenever Picture_Set_File_Data will return FALSE, then error will be reported by its calling function. Picture_Set_File_Data now accepts GError** as argument and will if at any stage an error occurred it will also set the passed GError. Also, I added g_assert statements so as to aid in debugging. Whenever, Picture_Set_File_Data returns FALSE, it will also set GError otherwise if it returns TRUE GError will be NULL. Please let me know what you think of this??
Review of attachment 243847 [details] [review]: Purely cosmetic problems now. For the commit message, simplify it to: "Use GFile rather than stdio in picture.c Fixes bug 700050" Also, I see that there is one more case of using fopen in picture.c. It would be nice to replace that with GFile or some better way of checking of the file exists before writing. ::: src/picture.c @@ +728,3 @@ + Log_Print (LOG_ERROR, + _("Image file '%s' cannot be saved: (%s)"), + filename_utf8, error->message); Check your indentation here. @@ +738,3 @@ + Log_Print (LOG_ERROR, + _("Image file '%s' cannot be saved: (%s)"), + filename_utf8, error->message); Check your indentation here too. @@ +1195,3 @@ }else { + /*Image Loaded*/ An extra space of indentation.
Created attachment 243880 [details] [review] oh!! corrected that one pending case and indentation problem
Review of attachment 243880 [details] [review]: ::: src/picture.c @@ +705,2 @@ // Warn user if the file already exists, else saves directly + if (g_file_query_exists (file, NULL)) It is racy to check that a file exists and then create it, as you can tell be reading the g_file_query_exists() documentation. You should probably call g_file_create() and then pass the GFile to Picture_Save_File_Data() (making it accept a GFile instead of a filename).
Created attachment 243882 [details] [review] Changed Picture_Save_File_Data to accept GFile instead of gchar * Wouldn't it be better to have the same behaviour over both functions, Picture_Save_File_Data and Picture_Open_File_Data. Please let me know if this should be done. Although, I did changed the behaviour of Picture_Save_File_Data.
Review of attachment 243882 [details] [review]: (In reply to comment #26) > Wouldn't it be better to have the same behaviour over both functions, > Picture_Save_File_Data and Picture_Open_File_Data. Please let me know if this > should be done. Yes, you should change Picture_Load_File_Data() to accept a GFileInputStream (or GInputStream) argument to make it similar to Picture_Save_File_Data(). ::: src/picture.c @@ +705,2 @@ // Warn user if the file already exists, else saves directly + if (g_file_query_exists (file, NULL)) As I said in my previous comment, using g_file_query_exists() to check if the file exists before writing to it introduces a race condition. Create the file with g_file_create() and then pass the GFileOutputStream to Picture_Save_File_Data().
Created attachment 243972 [details] [review] I changed Picture_Load_File_Data function to accept GFileOutputStream and URI's will be used to create GFile in Picture_Load_Filename. Also, changed Picture_Save_File_Data. Here's one problem, these line numbers are after applying the patch In function Tag_Area_Picture_Drag_Data, Line number 99. There is a "uri" which is first converted to filename and then passed to Picture_Load_Filename. Picture_Load_Filename now works using uri. So, it converts filename to uri. So, here first uri is converted to filename and then vice versa. Instead that uri should be used. Hence, a new function to accept uri (say Picture_Load_uri) could be created and Picture_Load_Filename could call this function after converting filename to uri. But, for error messages a filename is required to display hence, Picture_Load_uri will convert uri to filename. Here, again 2 times conversion happens. It may be done that Picture_Load_uri will return gboolean and Picture_Load_Filename will show the message box of error. But in the above line 99. If Picture_Load_uri is called then no error will be displayed. So, I think its better to leave it the way it is.
Review of attachment 243972 [details] [review]: (In reply to comment #28) > Created an attachment (id=243972) [details] [review] > I changed Picture_Load_File_Data function to accept GFileOutputStream and URI's > will be used to create GFile in Picture_Load_Filename. Also, changed > Picture_Save_File_Data. > > Here's one problem, these line numbers are after applying the patch > In function Tag_Area_Picture_Drag_Data, Line number 99. > There is a "uri" which is first converted to filename and then passed to > Picture_Load_Filename. Picture_Load_Filename now works using uri. So, it > converts filename to uri. So, here first uri is converted to filename and then > vice versa. Instead that uri should be used. Hence, a new function to accept > uri (say Picture_Load_uri) could be created and Picture_Load_Filename could > call this function after converting filename to uri. But, for error messages a > filename is required to display hence, Picture_Load_uri will convert uri to > filename. Here, again 2 times conversion happens. > It may be done that Picture_Load_uri will return gboolean and > Picture_Load_Filename will show the message box of error. But in the above line > 99. If Picture_Load_uri is called then no error will be displayed. > So, I think its better to leave it the way it is. Rather than interconverting between URIs and filenames, it is probably better to just pass GFiles around, especially as it is possible to get GFiles directly from the file chooser. From a GFile, you can get the basename (the last component of the filename) or the path, as well as fetch the name for display with g_file_query_info(). ::: src/picture.c @@ +838,3 @@ + g_error_free (error); + } + if (!g_output_stream_close (G_OUTPUT_STREAM (file_ostream), You have already closed the stream in Picture_Save_File_Data(), unless the write failed, so do not close it again here.
(In reply to comment #29) > Rather than interconverting between URIs and filenames, it is probably better > to just pass GFiles around, especially as it is possible to get GFiles directly > from the file chooser. From a GFile, you can get the basename (the last > component of the filename) or the path, as well as fetch the name for display > with g_file_query_info(). I started on this to give you an idea of how I think it should be done: https://git.gnome.org/browse/easytag/commit/?id=ef7666bddfe92ccbdcd1eea48da96f538696d42c
Created attachment 244131 [details] [review] Changed et_picture_load_file_data and Picture_Save_File_Data functions Thanks for that :) I changed et_picture_load_file_data and Picture_Save_File_Data functions to accept GFile*Stream, as it was decided earlier. Also, regarding your previous to previous comment about closing stream outside Picture_Save_File_Data. I decided to close the stream there only not inside Picture_Save_File_Data, because I think its better that the function which opens up a stream should be responsible for closing it not the one using that stream.
Review of attachment 244131 [details] [review]: (In reply to comment #31) > Created an attachment (id=244131) [details] [review] > Changed et_picture_load_file_data and Picture_Save_File_Data functions The patch does not apply. You should update from master and then redo your patch. > I changed et_picture_load_file_data and Picture_Save_File_Data functions to > accept GFile*Stream, as it was decided earlier. I think that accepting a GFile is better in this case. > Also, regarding your previous to previous comment about closing stream outside > Picture_Save_File_Data. I decided to close the stream there only not inside > Picture_Save_File_Data, because I think its better that the function which > opens up a stream should be responsible for closing it not the one using that > stream. Another reason why it would be better to pass a GFile and then open a stream in each of the load/save functions is that the stream can be opened and closed within the same function.
Created attachment 244138 [details] [review] Changed et_picture_load_file_data and Picture_Save_File_Data functions Both these functions accept GFile and GError as their arguments. I have shifted the message box at line 720 to Picture_Save_File_Data function, if done in previous way there would be race condition as discussed in comment 27 (and its preceding 2-3 comments). So, Picture_Save_File_Data will also check for the existence of file and will display the message box also.
Review of attachment 244138 [details] [review]: ::: src/picture.c @@ +264,3 @@ + GTK_BUTTONS_CLOSE, + _("Cannot open file: '%s'"), + filename_utf8); You need to indent the arguments by an extra space. @@ +266,3 @@ + filename_utf8); + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG(msgdialog), + "%s", g_strerror(errno)); Rather than using errno, get the error message from the GError passed to et_picture_load_file_data() @@ +728,1 @@ + if (!Picture_Save_File_Data (pic, file, &error) && error != NULL) You do not need to check that error has been set, as Picture_Save_File_Data always sets an error upon failure (or at least it should do, see my comment on that function). @@ +1175,2 @@ { + if ((*error)->code == G_IO_ERROR_EXISTS) Use g_error_matches() to check if the error matches a specific code and domain. @@ +1193,3 @@ + + filename_utf8 = g_file_info_get_display_name (info); + g_object_unref (info); As the string pointed to by filename_utf8 is owned by the GFileInfo, you should not unref the GFileInfo until after you have used filename_utf8. @@ +1200,3 @@ + GTK_BUTTONS_NONE, + _("The following file already exists: '%s'"), + filename_utf8); An extra space is needed before the arguments @@ +1216,3 @@ + { /*Response is GTK_RESPONSE_NO*/ + g_assert (error == NULL || *error == NULL); + return FALSE; According to the GError documentation, if a GError-using function returns FALSE, it _must_ also set the error appropriately. You either need to add a new error domain and code or find an existing one that is suitable. @@ +1223,3 @@ + { + g_assert (error == NULL || *error != NULL); + return FALSE; You need to unref the output stream before returning. @@ +1233,1 @@ return FALSE; You need to unref the output stream before returning. @@ +1237,3 @@ + { + g_assert (error == NULL || *error != NULL); + return FALSE; You need to unref the output stream before returning.
Created attachment 244160 [details] [review] Did all the changes The error domain used is G_IO_ERROR and code is G_IO_ERROR_CANCELLED. I included the message also in it.
Review of attachment 244160 [details] [review]: I pushed a modified patch to master as commit d0d458325bc40bbb82da4fc37dc81a64a930333d, and improved it in commit edcdc1db08d74af96f52a25c0985a92d79f25080. For future patches, as there is a fair bit of review needed, you should file a new bug per patch and set it to block this bug.
I ported flac_header.c to use GIO in master as commit 12ba8ac010d8b20900f323a19aaa6eab649e617c.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/easytag/-/issues/ Thank you for your understanding and your help.