Make the matching rule of printer device path more flexible by hugh712 · Pull Request #183 · OpenPrinting/system-config-printer

hugh712 · 2020-09-30T03:56:17Z

Update the udev rule due to usb_id udev import is disabled in some systems.
Make the matching rule of printer device path more flexible.

… systems. * Make the matching rule of printer device path more flexible.

zdohnal · 2020-09-30T14:19:32Z

I'm sorry for not getting to your issue yesterday, but I managed to look into the code on your fork and I would like to comment on it tomorrow.

Anyway, thank you for the PR!

zdohnal

Hi @hugh712 ,

I have a following comments about the code, would you mind looking into them?

Thank you in advance!

Zdenek

zdohnal · 2020-10-05T08:20:13Z

+  full_path = (char *) malloc(strlen(uri_store) + 4); 
+  memset(full_path, 0, sizeof(full_path));
+  strcpy(full_path, "/sys");
+  strcat(full_path, uri_store);


You can do this with single snprintf() call and it will take care of NULL terminator too. Or Am I missing something?

that's fine, I will do it

zdohnal · 2020-10-05T09:31:15Z

+  // check the exist of this device
+  if (subset == 1 && stat(full_path, &buffer) == 0){ 
+   exist = 1;  


I would propose to move it to a separate function - it is against the name of function. The name indicates it will compare the uris, not checking its existence.

device_exists() can be an additional argument in if (compare_usb_uri()) condition. IMO it causes no harm if it is run even on devices which aren't substrings, but they match will usb uri from udev event. What do you think?

zdohnal · 2020-10-05T09:32:24Z

+
+  // check whether it's a substring
+  full_path = (char *) malloc(strlen(uri_store) + 4); 
+  memset(full_path, 0, sizeof(full_path));
+  strcpy(full_path, "/sys");
+  strcat(full_path, uri_store);


Plus I would move this to separate function - device_exists().

zdohnal · 2020-10-05T09:47:00Z

+  // check whether it's a substring
+  full_path = (char *) malloc(strlen(uri_store) + 4); 


Please comment the magic number in a comment, thank you!

zdohnal · 2020-10-05T09:49:22Z

+  {
+      if (compare_usb_uri(entry->devpath, devpath))


What about using strstr() instead of strcmp(). It will do the trick with finding an exact match and substring too.

Then you can add device_exists() to check if the device really exists and move the code I marked into it.

zdohnal · 2020-10-05T09:51:23Z

 ACTION=="add", SUBSYSTEM=="usb", ENV{DEVTYPE}=="usb_device", ENV{ID_USB_INTERFACES}=="*:0701??:*", TAG+="systemd", ENV{SYSTEMD_WANTS}="configure-printer@usb-$env{BUSNUM}-$env{DEVNUM}.service"
 # Low-level USB device remove trigger
-ACTION=="remove", SUBSYSTEM=="usb", ENV{ID_USB_INTERFACES}=="*:0701*:*", RUN+="udev-configure-printer remove %p"
+ACTION=="remove", SUBSYSTEM=="usb", ENV{INTERFACE}=="7/1/*", ENV{INTERFACE}!="7/1/4", RUN+="udev-configure-printer remove %p"


Would you mind explaining about values of env variable INTERFACE? Why value 7/1/4 is omitted? It would be good to have a brief comment in the code too.

seems this is a trick from ubuntu, because all of the ippusbxd protocol (7/1/4) devices has been taken care by another rules, I think let's just leave this file as the original status, I will revert this one.

I will still keep ENV{INTERFACE}=="7/1/*" in this file, due to in systemd [0], it goes to default_end directly when removing action occurred, which meant IMPORT{builtin}="usb_id" will not be executed, so we have to use ENV{INTERFACE} when removing usb printer.

[0] https://github.com/systemd/systemd/blob/master/rules.d/50-udev-default.rules.in

zdohnal · 2020-10-06T10:55:24Z

Did you test the PR if it works? I tried to apply my suggestions into the code, but the print queue wasn't disabled - so I'm trying to find out whether I made a mistake or the design needs adjusting.

hugh712 · 2020-10-06T12:49:25Z

Did you test the PR if it works? I tried to apply my suggestions into the code, but the print queue wasn't disabled - so I'm trying to find out whether I made a mistake or the design needs adjusting.

Yes, I tried it and it works well

hugh712 · 2020-10-06T14:26:00Z

Hi @hugh712 ,

I have a following comments about the code, would you mind looking into them?

Thank you in advance!

Zdenek

Hi

Hi @hugh712 ,

I have a following comments about the code, would you mind looking into them?

Thank you in advance!

Zdenek

@zdohnal,
Hi, I have limited hardware access this week,
will work on it once I have the hardware, many thanks :)

zdohnal · 2020-10-06T14:32:48Z

No problem.

Use strstr to search substring. Use snprintf to combine the device path. Do not skip ippusbxd printer in the rules

hugh712 · 2020-10-13T04:48:54Z

@zdohnal,

Please help to review again, thanks :)

hugh712 · 2020-10-13T05:00:16Z

@zdohnal,

and please help to link this PR to the issue[0], thanks.

[0] #182

zdohnal · 2020-10-15T05:06:38Z

Hi @hugh712 ,

I'm sorry I won't probably get to the review this week. Hope I'll manage at the beginning of the next.

I'm sorry for inconvenience :(

hugh712 · 2020-10-21T03:38:11Z

@zdohnal,

Wondering do you have some spare time to review this PR this week? :)

zdohnal · 2020-10-21T05:24:48Z

Hi @hugh712 ,

I'm sorry, I haven't checked and tested it yet - I will not promise any exact date from now, but I have this PR on my TODO list. I'll give a heads-up when I get to this.

I'm really sorry for inconvenience and still I'm grateful for the PR!

hugh712 · 2020-10-21T09:11:48Z

no problem :)

tillkamppeter · 2020-11-02T16:37:59Z

@zdohnal, the ENV{INTERFACE}!="7/1/4" is to not treat interfaces which are for IPP-over-USB, as they are handled separately by the ipp-usb or ippusbxd daemon. They cannot be used by the "usb" backend of CUPS (uses 7/1/1 and 7/1/2) or the "hp" backend of HPLIP (uses 7/1/3).

tillkamppeter · 2020-11-02T16:39:54Z

@hugh712, thanks for your contribution, I have merged your pull request now.
If anyone finds corener cases which are not covered, please post an issue.

hugh712 · 2020-11-03T02:01:35Z

Great!, thank you so much :)

zdohnal · 2020-11-03T10:13:03Z

Thank you for merging the PR, Till!

@hugh712 thanks for the PR! I was able to test it at last and it works.

I'll do some minor changes regarding the code (spaces, use strstr directly...), but it looks good overall.

Again, sorry for the delay.

* Update the udev rule due to usb_id udev import is disabled in some…

d1aab4e

… systems. * Make the matching rule of printer device path more flexible.

zdohnal requested changes Oct 5, 2020

View reviewed changes

Seperate device_exists function from compare_usb_uri.

aa8bed0

Use strstr to search substring. Use snprintf to combine the device path. Do not skip ippusbxd printer in the rules

tillkamppeter merged commit 5d4faec into OpenPrinting:master Nov 2, 2020

tillkamppeter mentioned this pull request Nov 2, 2020

The status of cups still shows idle when the printer has been unplugged #182

Closed

		// check whether it's a substring
		full_path = (char *) malloc(strlen(uri_store) + 4);

Conversation

hugh712 commented Sep 30, 2020

Uh oh!

zdohnal commented Sep 30, 2020

Uh oh!

zdohnal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdohnal commented Oct 6, 2020

Uh oh!

hugh712 commented Oct 6, 2020

Uh oh!

hugh712 commented Oct 6, 2020

Uh oh!

zdohnal commented Oct 6, 2020

Uh oh!

hugh712 commented Oct 13, 2020

Uh oh!

hugh712 commented Oct 13, 2020

Uh oh!

zdohnal commented Oct 15, 2020

Uh oh!

hugh712 commented Oct 21, 2020

Uh oh!

zdohnal commented Oct 21, 2020

Uh oh!

hugh712 commented Oct 21, 2020

Uh oh!

tillkamppeter commented Nov 2, 2020

Uh oh!

tillkamppeter commented Nov 2, 2020

Uh oh!

hugh712 commented Nov 3, 2020

Uh oh!

zdohnal commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants