GNOME Bugzilla – Bug 766861
Only the first CUPS maintenance command is being checked
Last modified: 2016-08-02 18:52:59 UTC
At the moment, we are querying the printer for its 'printers-comands' attribute with a call to ippFindAttribute() and then extracting its value with ippGetString (attr, 0, NULL) to further search that string for occurrences of the maintenance command we want to execute. The problem is that the 'printers-comands' attribute returned is not a single value like the one that lpoptions -p would return if called from command line (e.g. 'AutoConfigure,Clean,PrintSelfTestPage'), but a list of several values instead, so that logic will only work for the 'AutoConfigure' command, which so far has been enough since it's the only one we've ever needed for a long time. We need to fix this now we will be adding support for the 'Clean' command, otherwise it won't be possible to use that feature, even if the printer supports it (e.g. Canon PIXMA MG2410 does, using cnijfilter drivers).
Created attachment 328483 [details] [review] [PATCH 1/3] printers: Removed unused include for pp-maintenance-command.h
Created attachment 328485 [details] [review] [PATCH 2/3] printers: Added new function pp_maintenance_command_is_supported()
Created attachment 328486 [details] [review] [PATCH 3/3] printers: Check all supported CUPS commands
Review of attachment 328485 [details] [review]: Thank you for looking at this. You are right we have to improve this part. ::: panels/printers/pp-maintenance-command.c @@ +224,1 @@ + if (response) Check this against NULL please @@ +305,3 @@ + ippAddString (request, IPP_TAG_OPERATION, IPP_TAG_URI, + "printer-uri", NULL, printer_uri); + ippAddStrings (request, IPP_TAG_OPERATION, IPP_TAG_KEYWORD, Use ippAddString here so that we don't need the attrs variable @@ +308,3 @@ + "requested-attributes", 1, NULL, attrs); + response = cupsDoRequest (CUPS_HTTP_DEFAULT, request, "/"); + if (response) Check against NULL please @@ +313,3 @@ + { + attr = ippFindAttribute (response, "printer-commands", IPP_TAG_ZERO); + if (attr && ippGetCount (attr) > 0 && ditto @@ +324,3 @@ + } + + if (printer_commands) ditto @@ +329,3 @@ + printer_commands_lowercase = g_ascii_strdown (printer_commands, -1); + + if (g_strrstr (printer_commands_lowercase, command_lowercase)) Use g_strcmp0() so we don't match just substring but whole string (so we are covered if there will be new command with "Clean" in its name) @@ +336,3 @@ + g_free (printer_commands); + } + You should free the printer_uri here ::: panels/printers/pp-maintenance-command.h @@ +64,3 @@ GError **error); +gboolean pp_maintenance_command_is_supported (const gchar *printer_name, This has to be run async in another thread so we don't block UI by the cupsDoRequest(). If you want to keep it here then you should make corresponding async/finish functions for checking whether the existing PpMaintenanceCommand is supported or wait until there will be PpPrinter class (see bug #761539) and create pp_printer_supports_command_async/finish ().
Review of attachment 328486 [details] [review]: ::: panels/printers/pp-maintenance-command.c @@ +315,2 @@ attr = ippFindAttribute (response, "printer-commands", IPP_TAG_ZERO); + commands_count = attr ? ippGetCount (attr) : 0; Check the attr against NULL please @@ +321,3 @@ + int i; + + available_commands = g_ptr_array_new (); You have to set free function by g_ptr_array_set_free_func() to be able to use the g_ptr_array_free() later @@ +333,3 @@ } + if (available_commands) Check this against NULL please @@ +336,2 @@ { + int i; Declare the i in the beginning of the function since you use it several times
Review of attachment 328483 [details] [review]: This patch breaks build of master branch
Thanks for the review, Marek. See my comments below... (In reply to Marek Kašík from comment #6) > Review of attachment 328483 [details] [review] [review]: > > This patch breaks build of master branch It seems this problem comes from the first patch of the set, which removed an unused include: I removed it because that was not needed in the baseline I used, based on Felipe's new-printers-panel branch), but it's definitely needed on master. So, dropping that patch will fix master (just checked now) (In reply to Marek Kašík from comment #4) > ::: panels/printers/pp-maintenance-command.c > @@ +224,1 @@ > + if (response) > > Check this against NULL please Ok. > @@ +305,3 @@ > + ippAddString (request, IPP_TAG_OPERATION, IPP_TAG_URI, > + "printer-uri", NULL, printer_uri); > + ippAddStrings (request, IPP_TAG_OPERATION, IPP_TAG_KEYWORD, > > Use ippAddString here so that we don't need the attrs variable I thought we had to use ippAddStrings() since we pass a list of requested attributes, but I've just checked and using ippAddString in this case (where the list has only 1 element) works fine too. Thanks for pointing it out. > @@ +308,3 @@ > + "requested-attributes", 1, NULL, attrs); > + response = cupsDoRequest (CUPS_HTTP_DEFAULT, request, "/"); > + if (response) > > Check against NULL please Ok. > @@ +313,3 @@ > + { > + attr = ippFindAttribute (response, "printer-commands", > IPP_TAG_ZERO); > + if (attr && ippGetCount (attr) > 0 && > > ditto Ok. > @@ +324,3 @@ > + } > + > + if (printer_commands) > > ditto Ok. > @@ +329,3 @@ > + printer_commands_lowercase = g_ascii_strdown (printer_commands, -1); > + > + if (g_strrstr (printer_commands_lowercase, command_lowercase)) > > Use g_strcmp0() so we don't match just substring but whole string (so we are > covered if there will be new command with "Clean" in its name) Ok. > @@ +336,3 @@ > + g_free (printer_commands); > + } > + > > You should free the printer_uri here Ooops! I actually added this in the second commit, so I think I just messed things up while rebasing. I'll fix it. > ::: panels/printers/pp-maintenance-command.h > @@ +64,3 @@ > GError > **error); > > +gboolean pp_maintenance_command_is_supported (const gchar > *printer_name, > > This has to be run async in another thread so we don't block UI by the > cupsDoRequest(). > If you want to keep it here then you should make corresponding async/finish > functions for checking whether the existing PpMaintenanceCommand is > supported or wait until there will be PpPrinter class (see bug #761539) and > create pp_printer_supports_command_async/finish (). I need to check bug 761539 before making a choice, I'll try to do this later this week. (In reply to Marek Kašík from comment #5) > Review of attachment 328486 [details] [review] [review]: > > ::: panels/printers/pp-maintenance-command.c > @@ +315,2 @@ > attr = ippFindAttribute (response, "printer-commands", > IPP_TAG_ZERO); > + commands_count = attr ? ippGetCount (attr) : 0; > > Check the attr against NULL please Ok. > @@ +321,3 @@ > + int i; > + > + available_commands = g_ptr_array_new (); > > You have to set free function by g_ptr_array_set_free_func() to be able to > use the g_ptr_array_free() later Ooops! Ok > @@ +333,3 @@ > } > > + if (available_commands) > > Check this against NULL please Ok. > @@ +336,2 @@ > { > + int i; > > Declare the i in the beginning of the function since you use it several times Ok. At the moment I have addressed all your comments but the one about making the new function async, since I need a bit more of time for it. In the meantime, feel free to check the updated commits in https://git.gnome.org/browse/gnome-control-center/log/?h=wip/msanchez/printers-clean-heads, although I'm planning to make update all the patches here later this week, with a complete solution. Thanks for the feedback
Created attachment 329698 [details] [review] printers: Added new async API to check availability of maintenance commands This cleans the code up a bit so that we can extract part of the logic from _pp_maintenance_command_execute_thread() before getting into fixing the problem in the logic checking whether a CUPS command is available. Besides, it will be useful to have this logic extracted as it will be used later on from pp-printer-entry.c to know whether the "Clean" command is available, in order to show a menu item "Clean Print Heads" (bug 764620).
Created attachment 329699 [details] [review] printers: Check all supported CUPS commands, not just the first one Use an array of strings to store every supported command and check the desired command against the elements in that list, instead of simply checking the first one.
Marek: See attached the new versions of the 2 proposed patches (just dropped the first one) taking into account all (I hope) the review comments mentioned above. In the end I decided to go for the threads-based approach as the new PpPrinter class seems to be targeting 3.24, while the new printers panel is for 3.22. If needed, we can always migrate the code later on, I guess, but for it would be great to have this functionality in already now, so I think it's worth the effort. Please let me know what you think, thanks!
Review of attachment 329698 [details] [review]: Thank you for the patch. But it needs some changes yet (mainly the use of GTask). I'm sorry for late response. ::: panels/printers/pp-maintenance-command.c @@ +194,3 @@ + { + ipp_t *request; + ipp_t *response = NULL; This pointer doesn't need to be initialised @@ +295,3 @@ + gboolean is_supported = FALSE; + ipp_t *request; + ipp_t *response = NULL; This pointer doesn't need to be initialised @@ +298,3 @@ + gchar *printer_uri; + gchar *printer_commands = NULL; + gchar *printer_commands_lowercase = NULL; ditto @@ +317,3 @@ + if (attr != NULL && ippGetCount (attr) > 0 && + ippGetValueTag (attr) != IPP_TAG_NOVALUE && + (ippGetValueTag (attr) == IPP_TAG_KEYWORD)) The brackets in the last part are redundant @@ +351,3 @@ + PpMaintenanceCommand *command = (PpMaintenanceCommand *) object; + PpMaintenanceCommandPrivate *priv = command->priv; + gboolean success = FALSE; You don't need to initialise this boolean @@ +365,3 @@ + GSimpleAsyncResult *res; + + res = g_simple_async_result_new (G_OBJECT (command), callback, user_data, pp_maintenance_command_is_supported_async); Use GTask please, GSimpleAsyncResult was deprecated in favor of GTask
Review of attachment 329699 [details] [review]: This patch looks good. Please push it after the first one will land into master.
(In reply to Mario Sánchez Prada from comment #10) > Marek: See attached the new versions of the 2 proposed patches (just dropped > the first one) taking into account all (I hope) the review comments > mentioned above. > > In the end I decided to go for the threads-based approach as the new > PpPrinter class seems to be targeting 3.24, while the new printers panel is > for 3.22. Actually, Martin has finished the patch and the new class is in master now.
Thanks for the review, I'll try to address those issues asap and provide new patches accordingly (In reply to Marek Kašík from comment #13) > > In the end I decided to go for the threads-based approach as the new > > PpPrinter class seems to be targeting 3.24, while the new printers panel is > > for 3.22. > > Actually, Martin has finished the patch and the new class is in master now. I see, although now I see the new class and the code written here I kind of feel this code fits better in PpMaintenanceCommand than in PpPrinter, so I'd leave it like proposed, if that's ok. Should you disagree, let me know and I'll do the changes as needed.
(In reply to Mario Sánchez Prada from comment #14) > I see, although now I see the new class and the code written here I kind of > feel this code fits better in PpMaintenanceCommand than in PpPrinter, so I'd > leave it like proposed, if that's ok. It's ok. Thank you
Created attachment 331120 [details] [review] printers: Added new async API to check availability of maintenance commands This cleans the code up a bit so that we can extract part of the logic from _pp_maintenance_command_execute_thread() before getting into fixing the problem in the logic checking whether a CUPS command is available. Besides, it will be useful to have this logic extracted as it will be used later on from pp-printer-entry.c to know whether the "Clean" command is available, in order to show a menu item "Clean Print Heads" (bug 764620).
Created attachment 331121 [details] [review] printers: Check all supported CUPS commands, not just the first one Use an array of strings to store every supported command and check the desired command against the elements in that list, instead of simply checking the first one.
Created attachment 331122 [details] [review] Migrate the remaining bits of PpMaintenanceCommand to GTask They were still using the deprecated GSimpleAsyncResult and throwing a lot of warnings when building.
Marek: Sorry for the slow response, been quite busy and struggled to find the right time to sit down and finish this, but hopefully is all set by now. See attached a reworked version of the first patch using GTask plus third new patch on to to migrate the remaining bits of PpMaintenanceCommand to GTask too which, although unrelated, I think makes sense as a separate commit considering I was already touching that file anyway. Notice I've updated the second patch too but that's only because I rebased the whole patch set on top of master. Other than that the patch is identical to the previously version. Please review and let me know whether this is good to commit now. Thanks!
Ping?
Review of attachment 331120 [details] [review]: Thank you for the conversion to GTask.
Review of attachment 331121 [details] [review]: Looks good to me.
Review of attachment 331122 [details] [review]: Thank you for the patch. It looks good to me too. Just add URL of this bug to the description for reference please.
Thanks for the review, Marek. I just landed them now: https://git.gnome.org/browse/gnome-control-center/commit/?id=4b91972f2c54b7071b9a1603a73eda00685a2282 https://git.gnome.org/browse/gnome-control-center/commit/?id=c41bbfa7d0cd3c16e6ac144e8030bb60c6341053 https://git.gnome.org/browse/gnome-control-center/commit/?id=bbe6b6309d206bfe257bb142b8a24b0d60ddd9f3 Resolving as fixed