[Desktop_printing] CUPS code quality ...
imcdonald at sharplabs.com
Thu Feb 2 11:00:59 PST 2006
[for context] PAPI is the FSG Open Printing standard PAPI/1.0 (July
There have been complete open source reference implementations for
several years. Norm Jacobs (Sun, chair of FSG Open Printing) has
PAPI supporting IPP and LPR on Solaris and is the principal current
maintainer of the PAPI specification and source.
Input for extensions to PAPI/1.0 is welcome (we're looking at
Capabilities now, which means PPD parsing, etc.).
- Ira (for FSG/OP International Steering Committee)
Ira McDonald (Musician / Software Architect)
Blue Roof Music / High North Inc
PO Box 221 Grand Marais, MI 49839
email: imcdonald at sharplabs.com
> -----Original Message-----
> From: desktop_printing-bounces at lists.osdl.org
> [mailto:desktop_printing-bounces at lists.osdl.org]On Behalf Of michael
> Sent: Thursday, February 02, 2006 12:24 PM
> To: Michael Sweet
> Cc: desktop_printing at lists.osdl.org
> Subject: Re: [Desktop_printing] CUPS code quality ...
> Hi Michael,
> Good name incidentally :-)
> On Thu, 2006-02-02 at 08:35 -0500, Michael Sweet wrote:
> > I'll address these points in reverse:
> Well, it seems you're at beginning to consider some of
> these issues in
> 1.2 that's great.
> > 1. Thread safety; CUPS 1.2 is thread-safe, and
> threading support
> > is enabled by default. Previous releases were
> thread safe if
> > you stayed away from the convenience functions that
> used shared
> > global data.
> Ok - well, I guess thread local storage is one way
> around returning
> const static internal buffers - although the gotcha for the
> unwary still
> remains. Clearly having a unified, standard calling convention for
> methods helps substantially wrt. learning the API, not
> and auto-generated scripting bindings to name a few.
> > 2. Synchronous; only if you use the convenience APIs -
> we use the
> > CUPS API as the basis for cupsd, and all of the HTTP and IPP
> > APIs operate as FSMs so that we can handle multiple
> > asynchronously. Other apps can do this or use
> threads as they
> > like.
> So - can you write me a short code-fragment, with error
> partial write support etc. that would asynchronously fetch a PPD ? (no
> commenting necessary ;-) [ also some idea of how to integrate
> that into
> a custom mainloop - just need a socket to poll on etc. ] [
> I'm assuming
> here there is an API function to handle a partial read in
> response to a
> POLLIN|POLLPRI input, and O_NONBLOCK socket handling ? ]
> > 2a. Variable time; them's the breaks, some people have fast
> > systems, some slow, some people use local servers, some
> > remote, some people have 1 printer, some have 10000.
> Sure - so clearly synchronous APIs are a disaster
> waiting to happen -
> particularly without threading support; but of course this is
> now fixed
> in 1.2 :-) [ which I don't seem to have on my bleeding-edge
> system: odd,
> so if you don't mind I'll refer below to what I see in 1.1 ].
> > 4. Badly designed; also a matter of opinion, although
> I heartily
> > agree that certain parts of the API are showing
> their age and
> > could use a redesign. This, unfortunately, is a challenge
> > with the parallel goal of maintaining backwards
> > in the 1.x series, but we welcome feedback in improving the
> > existing APIs and adding new ones that match
> developer usage.
> Of course back-compat (particularly with this legacy)
> is a pain; but -
> then the problems appear to be legion - an entirely new API (perhaps
> built on top of CUPS initially) would not have this problem of course.
> > You *do* know that you can do a CUPS_GET_PRINTERS request with a
> > non-blocking HTTP connection to collect the data as you go along,
> > right? And use requested-attributes to reduce the amount of data
> > that is processed?
> No - I have no idea; it's not clear from the headers,
> it's not clear to
> me how to do it. When I read the code in days of yore & the
> IPP spec. it
> was not clear to me how to construct arbitrary
> strings/URIs/etc./whatever & use the raw CUPS http code to achieve
> > Comments like this are not useful. Simple documentation = easier to
> > follow code (and clearly you are able to follow the CUPS
> code, even if
> > you don't like the design...)
> I spend my life reading code of often very mixed
> quality - some of the
> worst is unfortunately my own :-) perhaps you'd accept a patch to CUPS
> from me ? ;-)
> > We use static and dynamic buffers when they make sense.
> > Sockets are a requirement for using IPP over HTTP.
> When does a static buffer make sense ? seriously; this
> is my point -
> network traffic is slow, IPC is slow - strdup() is
> blindingly, amazingly
> fast. My point is - why use static buffers all over the place
> (which may
> give some marginal speed win) when they also tend to have well known
> security issues and of course introduce arbitrary fixed
> limits all over
> the show: thus either wasting memory unnecessarily or limiting
> longer-string corner cases.
> > It is synchronous by design, but nothing prevents you from using the
> > HTTP and IPP APIs to do this asynchronously - you'll just need to
> > do the IPP queries and then the HTTP GET yourself.
> So again; when I tried to do this some year ago - it
> was extremely
> unclear how to do that; but perhaps I was just missing
> something simple.
> > > Of course, what the contract is wrt. this file -
> whether you get to
> > > unlink it later at your leisure, or what is being really
> passed to you
> > > is (AFAICS) not explicit in the commenting. An opaque handle with
> > > clearly defined & consistent lifecycle semantics would clearly be
> > > better.
> > We're not trying to define a whole new object/abstraction interface
> > here (i.e. no glib or similar frameworks), just trying to get the
> > job done.
> Well; I'm not asking for rocket science here - just
> expecting some of
> the hard-learned lessons of maintaining stable ABIs over long
> periods to
> hit home.
> So - lets take another example:
> typedef struct
> int fd; /* File descriptor for
> this socket */
> char fields[HTTP_FIELD_MAX][HTTP_MAX_VALUE];
> /**** New in CUPS 1.1.19 ****/
> fd_set *input_set; /* select() set for
> httpWait() */
> http_status_t expect; /* Expect: header */
> char *cookie; /* Cookie value(s) */
> /**** New in CUPS 1.1.20 ****/
> char authstring[HTTP_MAX_VALUE],
> /* Current
> Authentication value */
> /* Username:password string */
> int digest_tries; /* Number of tries for
> digest auth */
> } http_t;
> So several things to say here: exposing all this lot:
> * gives you no guarantees of encapsulation, anything can
> screw with any of the fields at any time in any thread,
> in uncontrolled ways; resulting in inconsistent state.
> * gives you expansion problems: yes, I'm sure you're not
> expecting people to stack/heap allocate this structure
> themselves (when compiling with some old headers) - but
> someone, somewhere is going to do this if they can.
> * gives you hard-to-detect versioning problems; if I
> compile vs. 1.1.20 and run with 1.1.19 - and try to
> access 'authstring' which was there in the header when
> I compiled: what happens ?
> * the alternative being a linker error when an
> non-available accessor is called; or a named /
> enumerated property is not available.
> A far better way is to have an opaque structure, and either
> named/enumerated properties - or individual accessors. If you're
> particularly 31337 you can not dup strings and return 'const char*'
> pointers to the internal representation, thus saving a
> handful of cycles
> and exposing yourself to a different set of problems later :-)
> Things like:
> char fields[HTTP_FIELD_MAX][HTTP_MAX_VALUE];
> are particularly odd; HTTP_FIELD_MAX is 28 for me ( the
> end of an
> enumeration ) of course - it's not really possible to extent that
> enumeration without breaking the entire http_t layout ;-) but
> that aside
> we have:
> # define HTTP_MAX_URI 1024 /* Max length
> of URI string */
> # define HTTP_MAX_HOST 256 /* Max length
> of hostname string */
> # define HTTP_MAX_BUFFER 2048 /* Max length of data buffer */
> # define HTTP_MAX_VALUE 256 /* Max header field
> value length */
> So fields consumes 7kb, and other bits perhaps another
> 2 1/2k or so,
> size-wise not important, but most of that will be wasted most of the
> > > Is this symptomatic of a wider problem ? no idea. All I
> know is my
> > > experience of using the client code & trying to make it do what is
> > > necessary has been poor in the extreme: of course, it's entirely
> > > possible that is just down to my ignorance.
> > In my experience, too many developers jump to conclusions about
> > software before asking the people who wrote the software...
> Well - fine; what is your ABI compatibility strategy
> here ? why is
> there no encapsulation/data-hiding ? why are there all these arbitrary
> limits around the place ? can a FIELD really not contain a
> URI ? ( field
> max 256chars, URI max 1024 chars ).
> Of course perhaps my critique is unfair - I've only had
> to use the API
> in quite a minor way; but the above are my questions from reading the
> http.h header; from a quick scan similar problems are elsewhere too.
> > There is already an API like this: PAPI. However, you may be
> > disappointed to discover that PAPI's APIs are all
> synchronous, with no
> > possibility of asynchronous operation. PAPI also suffers from the
> > "least common denominator" syndrome, but that will likely
> improve over
> > time...
> Interesting; of course - as you see I'm almost totally
> ignorant of
> printing as a domain :-) perhaps PAPI is what I'm looking
> for. ie. if I
> was going to bless an API/ABI to have to support for the next
> 20 years &
> remain compatible with - I would not be picking CUPS in it's current
> state. Of course as you say, that's a personal opinion, and
> perhaps I'm
> deluded & you'll persuade me otherwise :-) I look forward to that
> Thanks for listening,
> michael.meeks at novell.com <><, Pseudo Engineer, itinerant idiot
More information about the Printing-summit