cups-browsed: Added AllowResharingRemoteCUPSPrinters directive to cups-browsed #218

vikrantmalik051 · 2020-03-12T09:09:06Z

Hi Till,

The directive NewBrowsePollQueuesShared which shares printers discovered by the browse poll directive is added in the method browse_poll_get_printers as here it was already being checked for printers discovered by BrowsePoll directive.
Kindly let me know if this patch requires any modifications.

tillkamppeter · 2020-03-12T11:07:15Z

utils/cups-browsed.c

+	    num_options = cupsAddOption("printer-is-shared", "true",
+				    num_options, NULL);
+	    debug_printf("Setting printer-is-shared bit.\n");
+      }
    }


To which option set are you adding the "printer-is-shared" option here? "cupsAddOption()" needs the option set to add the option to as its 4th argument, where you set NULL. See the source code of cupsAddOption() in CUPS, file cups/options.c. Did you actually build and test this code?

@tillkamppeter , I added this commit to get reviews from you about the code and therefore, did not completely test the code. However, I have added a new commit, after studying more about sending requests and changing options, which would change the printer-is-shared option when required. Kindly let me know if anything needs to be changed.

This also is not the correct thing. Here you are setting the printer-is-shared bit on the remote print queue. Once this does not make sense as on the remote server all these printers are shared, otherwise you would not see them as a client. Also you are sending the request for setting the shared bit to your local CUPS, but the printers are on remote CUPS servers. The request will error.
Did you test what you are doing? With the new option set you should have your local queues for BrowsePolled printers shared, with the new option not set they should be not shared.

@tillkamppeter , I am facing trouble while testing the code. When I try to modify the attribute printer-is-shared of the queue on the client, it shows the error unauthorised. Even when trying to change the printer-is-shared bit from web interface, the error is
Cannot change printer-is-shared for remote queues. I really want to solve this issue but I wanted your help to figure out why remote queue, instead of local queue is appearing on the client side. On the cups-browsed.conf file on the client, the following options are set.
BrowseRemoteProtocols dnssd cups
BrowseInterval 60
BrowseTimeout 300
BrowsePoll 192.168.43.210:631
OnlyUnsupportedByCUPS Yes

What is the device URI of the local queue with which you get the error (run lpstat -v)? Does it have a PPD file in /etc/cups/ppd/?

The Device URI, which appears on the client is implicitclass://D1_172_20_10_7/ where D1 is the description of the printer on the server. There also exists the following ppd file for this printer.
ppd_file.txt

@tillkamppeter , I think my issue might be related to this issue and therefore, queues pointing to remote queues should not be changed. Could you please confirm this?

CUPS actually does not allow re-sharing print queues which point to a remote CUPS queue. CUPS recognizes these queues in two ways (search for |= CUPS_PRINTER_REMOTE in the scheduler/printers.c file of the CUPS source code):

The device URI is CUPS-typical, like for example ipp://host:631/printers/queue (resource is /printers/queue, not /ipp/print or /ipp/print/queue) or ipp://service_name._ipp._tcp.local/cups (resource /cups instead of no resource).

The PPD file contains *APRemoteQueueID: ....

Long time ago I followed the CUPS functionality by letting cups-browsed add a *APRemoteQueueID: ... line to the PPD. Simply search for "*APRemoteQueueID:" in utils/cups-browsed.c and comment out these lines for your further studies. Then restart cups-browsed. Now you should be able to share such a queue.
Note that all queues discovered by BrowsePoll are remote CUPS queues, so the cups-browsed as it is now will always add *APRemoteQueueID: ... to the PPD file.
There are two solutions: Either remove the code for adding *APRemoteQueueID: ... altogether or add a second boolean directive to cups-browsed.conf, call it "AllowResharingRemoteCUPSPrinters" and if set to "Yes" let the *APRemoteQueueID: ... not be added. Your directive will only take effect then.

vikrantmalik051 · 2020-03-18T11:01:58Z

add a second boolean directive to cups-browsed.conf, call it "AllowResharingRemoteCUPSPrinters" and if set to "Yes" let the *APRemoteQueueID: ... not be added.

@tillkamppeter , In that case, I've observed that print queues are automatically shared if AllowResharingRemoteCUPSPrinters is set to yes, may be because the option settings are copied of the shared printer. Therefore, there wouldn't be a need of the NewBrowsePollQueuesShared directive. But, do you think it should be added in case someone does not want to share the queues browsepolled from a server?
As for now, I've tested and added the code for AllowResharingRemoteCUPSPrinters directive.

vikrantmalik051 · 2020-03-19T01:51:35Z

@tillkamppeter , Should I squash the commits for this to be merged?

tillkamppeter · 2020-03-19T12:28:57Z

Note that all discovered remote CUPS queues are shared on their CUPS servers., otherwise they would not be discoverable. I do not know why now all client queues get shared when we allow re-sharing by the cups-browsed code. Perhaps this is a local default on your client.
The AllowResharingRemoteCUPSPrinters directive should only influence whether *APRemoteQueueID: ... is added to the PPD or not, and so whether the user can switch the printer-is-shared bit to the queue being shared.
We still need the NewBrowsePollQueuesShared directive, so that one has active control on whether a local queue newly created by cups-browsed triggered by BrowsePoll is shared or not. Default should be not shared. Please always actively set a new queue to shared or not shared, so that it follows this directive. Add to the documentation that the AllowResharingRemoteCUPSPrinters must be set to Yes so that new BrowsePoll queues can get shared.

vikrantmalik051 · 2020-03-19T14:51:26Z

I do not know why now all client queues get shared when we allow re-sharing by the cups-browsed code. Perhaps this is a local default on your client.

Apparently, they were shared because in update_cups_queues, the request is not sent to server as printer->netprinter is verified not to be 0. But if AllowResharingRemoteCUPSPrinters is set, the request is sent and is accepted without an error.

vikrantmalik051 · 2020-03-19T19:36:11Z

@tillkamppeter , I've tested and added the NewBrowsePollQueuesShared directive. I've used the option printer-to-be-shared and not printer-is-shared so that it doesn't create problems regarding creation/modification of local queues in update_cups_queues. Please let me know if any further changes are required.

tillkamppeter · 2020-03-19T22:06:05Z

Have you taken care that the NewBrowsePollQueuesShared directive should only influence local print queues based on remote CUPS printer discovery via BrowsePoll and not on local queues based on legacy CUPS broadcasting of remote CUPS queues by old-generation (1.5 and older) CUPS servers?

vikrantmalik051 · 2020-03-20T04:13:51Z

@tillkamppeter , I have taken care of that in my latest commit.

tillkamppeter · 2020-03-20T09:39:38Z

utils/cups-browsed.c

@@ -430,6 +430,8 @@ static unsigned int BrowseInterval = 60;
 static unsigned int BrowseTimeout = 300;
 static uint16_t BrowsePort = 631;
 static browsepoll_t **BrowsePoll = NULL;
+int NewBrowsePollQueuesShared = 0;


@V1krant, could you add the "static" here? Thanks.

Yeah sure. I forgot to add that during testing and added that now.

tillkamppeter · 2020-03-20T10:29:57Z

@V1krant, OK, we are shortly before merging your Pull Request. Could you adjust the indentation of your code with the already existing code, the tabs in the file should stand for 8 spaces, or better, use only spaces.
Could you then also squash your commits to one?
Did you also test your code for all situations?

vikrantmalik051 · 2020-03-20T10:59:47Z

@V1krant, OK, we are shortly before merging your Pull Request. Could you adjust the indentation of your code with the already existing code, the tabs in the file should stand for 8 spaces, or better, use only spaces.
Could you then also squash your commits to one?

Sure, I'll just update it and push the commit.

Did you also test your code for all situations?

I've tested it for all situations except for old versions of cups (for the HAVE_CUPS_1_6 flag).

vikrantmalik051 · 2020-03-20T11:11:31Z

@tillkamppeter , I've squashed the commits and replaced tabs with spaces. Kindly let me know if any further changes are required

utils/cups-browsed.c

…es to cups-browsed

tillkamppeter · 2020-03-20T14:12:00Z

@V1krant, thank you very much. I merged your work now.

ibaldonl · 2020-03-20T15:08:17Z

Hey! Thanks so much for your work guys!!! Thanks for contributing it. If you ever come to Uruguay let me know!

tillkamppeter reviewed Mar 12, 2020

View reviewed changes

vikrantmalik051 force-pushed the master branch from 6c019cf to 4b35a28 Compare March 15, 2020 17:28

vikrantmalik051 changed the title ~~cups-browsed: Added new NewBrowsePollQueuesShared directive to cups-browsed~~ cups-browsed: Added AllowResharingRemoteCUPSPrinters directive to cups-browsed Mar 18, 2020

tillkamppeter reviewed Mar 20, 2020

View reviewed changes

vikrantmalik051 force-pushed the master branch from b365999 to b47e97f Compare March 20, 2020 11:10

tillkamppeter reviewed Mar 20, 2020

View reviewed changes

utils/cups-browsed.c Outdated Show resolved Hide resolved

vikrantmalik051 force-pushed the master branch 4 times, most recently from 3158552 to 20ceb1f Compare March 20, 2020 13:59

Added NewBrowsePollQueuesShared and AllowResharingcupsQueues directiv…

b6cbd1f

…es to cups-browsed

vikrantmalik051 force-pushed the master branch from 20ceb1f to b6cbd1f Compare March 20, 2020 14:03

tillkamppeter merged commit 5f75a5b into OpenPrinting:master Mar 20, 2020

tillkamppeter mentioned this pull request Mar 20, 2020

NewBrowsePollQueuesShared option? #101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cups-browsed: Added AllowResharingRemoteCUPSPrinters directive to cups-browsed #218

cups-browsed: Added AllowResharingRemoteCUPSPrinters directive to cups-browsed #218

vikrantmalik051 commented Mar 12, 2020

tillkamppeter Mar 12, 2020

vikrantmalik051 Mar 12, 2020 •

edited

Loading

tillkamppeter Mar 12, 2020

vikrantmalik051 Mar 15, 2020 •

edited

Loading

tillkamppeter Mar 16, 2020

vikrantmalik051 Mar 16, 2020 •

edited

Loading

vikrantmalik051 Mar 17, 2020 •

edited

Loading

tillkamppeter Mar 17, 2020

vikrantmalik051 commented Mar 18, 2020

vikrantmalik051 commented Mar 19, 2020

tillkamppeter commented Mar 19, 2020

vikrantmalik051 commented Mar 19, 2020

vikrantmalik051 commented Mar 19, 2020

tillkamppeter commented Mar 19, 2020

vikrantmalik051 commented Mar 20, 2020

tillkamppeter Mar 20, 2020

vikrantmalik051 Mar 20, 2020 •

edited

Loading

tillkamppeter commented Mar 20, 2020

vikrantmalik051 commented Mar 20, 2020 •

edited

Loading

vikrantmalik051 commented Mar 20, 2020

tillkamppeter commented Mar 20, 2020

ibaldonl commented Mar 20, 2020

cups-browsed: Added AllowResharingRemoteCUPSPrinters directive to cups-browsed #218

cups-browsed: Added AllowResharingRemoteCUPSPrinters directive to cups-browsed #218

Conversation

vikrantmalik051 commented Mar 12, 2020

tillkamppeter Mar 12, 2020

Choose a reason for hiding this comment

vikrantmalik051 Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

tillkamppeter Mar 12, 2020

Choose a reason for hiding this comment

vikrantmalik051 Mar 15, 2020 • edited Loading

Choose a reason for hiding this comment

tillkamppeter Mar 16, 2020

Choose a reason for hiding this comment

vikrantmalik051 Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

vikrantmalik051 Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

tillkamppeter Mar 17, 2020

Choose a reason for hiding this comment

vikrantmalik051 commented Mar 18, 2020

vikrantmalik051 commented Mar 19, 2020

tillkamppeter commented Mar 19, 2020

vikrantmalik051 commented Mar 19, 2020

vikrantmalik051 commented Mar 19, 2020

tillkamppeter commented Mar 19, 2020

vikrantmalik051 commented Mar 20, 2020

tillkamppeter Mar 20, 2020

Choose a reason for hiding this comment

vikrantmalik051 Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

tillkamppeter commented Mar 20, 2020

vikrantmalik051 commented Mar 20, 2020 • edited Loading

vikrantmalik051 commented Mar 20, 2020

tillkamppeter commented Mar 20, 2020

ibaldonl commented Mar 20, 2020

vikrantmalik051 Mar 12, 2020 •

edited

Loading

vikrantmalik051 Mar 15, 2020 •

edited

Loading

vikrantmalik051 Mar 16, 2020 •

edited

Loading

vikrantmalik051 Mar 17, 2020 •

edited

Loading

vikrantmalik051 Mar 20, 2020 •

edited

Loading

vikrantmalik051 commented Mar 20, 2020 •

edited

Loading