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

[Jennifer Godinez]

Fix is pushed in both 8 and 7u40.

Jennifer

On 6/6/2013 1:18 PM, Phil Race wrote:
> 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.
>>
>