[OpenJDK 2D-Dev] JDK-8013810: UnixPrintServiceLookup not returning consistent values

[Patrick Reinhart]

Hi Phil,

Am 10.05.13 00:08, schrieb Phil Race:
> Patrick,
> Maybe you need to be clear in the problem statement. I can't actually
> find it anywhere in this email thread.  I'm reverse engineering to it 
> as follows:
>
> You got an IPPPrintService for lookup via
>
> PrintServiceLookup.lookupPrintServices(null, null)
>
>
> but a UnixPrintService via
>
> attributes.add(new PrinterName(name, null));
> PrintServiceLookup.lookupPrintServices(null, attributes)
>
>
That is absolutely correct. My problem is exactly as you described it above.
> If you read my email you should see that if you make the change
> I proposed you will never enter the getNamedPrinterXXX() methods
> if you are using CUPS, which seems like it should fix your problem
> of different classes.
When I look into your diff, then your intention becomes clear to me and 
yes, that will fix my problem. And from this point of view it's no need 
to call those getNamedPrinterXXX() methods.
> You don't really need to test the class and probably shouldn't.
> service.equals(serviceByName) seems better.
That's correct too and wouldn't change the tests purpose.
> And although getServiceByName() shouldn't enumerate all printers,
> because that can be very costly in some configurations,
> it likely should first check to see if we already enumerated all 
> printers,
> and then see if its in the list. As its written its really intended 
> for the
> case where no enumeration has been performed but if it already
> happened there's no harm in leveraging that.
That point I understood. What I do not understand though is the reason 
left for those methods anyway. What's the use case for them? Only to 
check the state in case of a non IPPPrintService, that is not being done 
if let's say a enumerated UnixPrintService is already available? Does a 
UnixPrintService this not by them-self already?
> Put another, way does this patch (on its own, no other changes) fix 
> your problem
> It modifies the getPrinterByName() method. If not, why, not, and 
> please print
> out the actual class/service names seen by your test so we can see 
> more clearly
>
> diff --git a/src/solaris/classes/sun/print/UnixPrintServiceLookup.java 
> b/src/solaris/classes/sun/print/UnixPrintServiceLookup.java
> --- a/src/solaris/classes/sun/print/UnixPrintServiceLookup.java
> +++ b/src/solaris/classes/sun/print/UnixPrintServiceLookup.java
> @@ -366,6 +366,29 @@
>          if (name == null || name.equals("") || 
> !checkPrinterName(name)) {
>              return null;
>          }
> +
> +        if (printServices != null) {
> +            for (int i=0; i<printServices.length; i++) {
> +                if (name.equals(printServices[i].getName())) {
> +                    return printServices[i];
> +                }
> +            }
> +        }
> +
> +        if (CUPSPrinter.isCupsRunning()) {
> +            try {
> +                URL url = new URL("http://"+
> +                                  CUPSPrinter.getServer()+":"+
> +                                  CUPSPrinter.getPort()+"/"+
> +                                  name);
> +                printer = new IPPPrintService(name, url);
> +            } catch (Exception e) {
> +            }
> +            if (printer != null) {
> +                return printer;
> +            }
> +        }
> +
>          if (isMac() || isSysV()) {
>              printer = getNamedPrinterNameSysV(name);
>          } else {
Is it on purpose to iterate over the enumerated printers not using the 
for-each loop?
> BTW keep all revisions of your code around so that people can compare.
> I have no reference to what the earlier version of your fix did.
I will keep that in mind for my next changes...

Besides all that, It seems to me a much of code duplication even though 
it works. I would prefer to have that tighten up a bit too.

Patrick

> -phil.
>
> On 5/8/2013 12:48 AM, Patrick Reinhart wrote:
>> Hi Phil,
>>
>> Sorry, I can not share your point of the methods 
>> getNamedPrinterNameXX() always returning a UnixPrintService though. 
>> That is exactly the point of inconsistency shown by the test case. As 
>> far as I checked it the only referred places of those 
>> getNamedPrinterNameXX() is exactly the getServiceByName() method 
>> mentioned to place the fix.
>>
>> In my opinion those method names are misleading anyway. They both 
>> called getNamedPrinterNameXX() even though they return a 
>> PrintService. As I assume that they originally did return the active 
>> printer name and the PrintService was created outside, as the usage 
>> of the getAllPrinterNamesXX() methods - that are called when creating 
>> all PrintService instances. You explained me before that in case of 
>> hundreds of printers it's faster to just lookup the printer state of 
>> one printer only - which is done within the getNamedPrinterNameXX()  
>> methods.
>>
>> Patrick
>>
>>
>> Am 08.05.13 01:38, schrieb Phil Race:
>>> I am assuming this  current webrev replaced the previous
>>> one:- http://reinharts.dyndns.org/webrev/
>>>
>>> getNamedPrinterNameSysV() and
>>>
>>> getNamedPrinterNameBSD()
>>>
>>> should not be changed as they should always
>>> create a UnixPrintService and so can do this
>>> directly. You are subverting the code in
>>> there to first check for CUPS which shouldn't
>>> be needed.
>>>
>>>
>>> I haven' t tested this out but from inspection,
>>> I think the problem is in getServiceByName() where
>>> it is ignorant of the CUPS possibility.
>>> It needs to have the isCUPSRunning() check and
>>> if so create an IPPPrintService().
>>>
>>> So that's where you should place your call.
>>>
>>> That is likely the only change here that is really necessary.
>>> The refactoring is OK but but not essential to the fix.
>>>
>>> -phil.