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

[Phil Race]

Hello,

The fix looks fine since appears to be functionally equivalent to
what I suggested in whatever was my last email.

In terms of back porting I think we literally have until Tuesday
to get it into 7u40.

Jennifer : can you handle the logistics ?

-phil.

On 5/10/2013 6:15 AM, Patrick Reinhart wrote:
> 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.
>