On Thu, Jul 01, 2010 at 01:12:11PM +0200, Michael Hanselmann wrote:
> Am 30. Juni 2010 19:14 schrieb Iustin Pop <[email protected]>:
> > On Wed, Jun 30, 2010 at 06:04:56PM +0200, Michael Hanselmann wrote:
> >> There's one small drawback: Programs using libcurl have to call
> >> curl_global_init(3) (available as pycurl.global_init) when only
> >> one thread is running and are supposed to call
> >> curl_global_cleanup(3) (available as pycurl.global_cleanup) upon
> >> exiting. A decorator is provided to simplify this.
> >
> > What do you mean "when only one thread is running"? Can we skip this
> > step in multithreaded programs?
>
> In short: it needs to called in all programs. In multi-threaded
> programs before starting any thread. Not calling global_init causes
> libcurl to do so automatically when doing the first transfer, which
> might be fine in single-threaded programs, but not when more than one
> thread is running.
>
> curl_global_init(3), referenced by the docstring, describes it in more
> detail: “This function must be called at least once within a program
> (a program is all the code that shares a memory space) before the
> program calls any other function in libcurl. […] This function is not
> thread safe. You must not call it when any other thread in the program
> (i.e. a thread sharing the same memory) is running. This doesn't just
> mean no other thread that is using libcurl. Because curl_global_init()
> calls functions of other libraries that are similarly thread unsafe,
> it could conflict with any other thread that uses these other
> libraries.” (http://curl.haxx.se/libcurl/c/curl_global_init.html)
Thanks. Can you just change in the commit msg "when only one thread is
running" to "before starting any threads"?
> >> - �...@staticmethod
> >> - def _VerifySslCertCb(logger, _, cert, errnum, errdepth, ok):
> >> - """Callback for SSL certificate verification.
> >> -
> >> - �...@param logger: Logging object
> >> -
> >> - """
> >> - if ok:
> >> - log_fn = logger.debug
> >> + logger.debug("Using cURL version %s", pycurl.version)
> >> +
> >> + sslver = _pycurl_version_fn()[5]
> >
> > this seems a bit cryptic; could you add either a comment with that that
> > function should return, or a comment with the actual function being
> > used? (I had to dig it up from the keyword args to this function).
>
> Done:
> + # pycurl.version_info returns a tuple with information about the used
> + # version of libcurl. Item 5 is the SSL library linked to it.
> + # e.g.: (3, '7.18.0', 463360, 'x86_64-pc-linux-gnu', 1581,
> 'GnuTLS/2.0.4',
> + # 0, '1.2.3.3', ...)
LG.
> >> - handlers = [_HTTPSHandler(self._logger, config_ssl_verification)]
> >> -
> >> + # Create pycURL object if not supplied
> >> + if not curl:
> >> + curl = pycurl.Curl()
> >> +
> >> + # Default cURL settings
> >> + curl.setopt(pycurl.VERBOSE, False)
> >> + curl.setopt(pycurl.FOLLOWLOCATION, True)
> >
> > why followlocation? we shouldn't have this in ganeti rapi daemon, right?
>
> Not at the moment, but I'd rather not have old(er) clients break just
> because we started using redirects for some of the resources they use.
> The number of redirects is limited:
>
> >> + curl.setopt(pycurl.MAXREDIRS, 5)
Yes, but still & AFAIK, redirects can be evil (e.g. redirects and curl
re-sending authentication etc.).
Meh, up to you.
> > […]
> >> + # Get HTTP response code
> >> + http_code = curl.getinfo(pycurl.RESPONSE_CODE)
> >> +
> >> + if encoded_resp_body.tell():
> >> + response_content = simplejson.loads(encoded_resp_body.getvalue())
> >> else:
> >> response_content = None
> >
> > I'm a bit confused how this tell() works… Can you clarify?
>
> It checks whether anything was written to the buffer. Added a comment
> to this effect.
LG, thanks.
> >> + def testCertVerifyCurlBundle(self):
> >> + cfgfn = client.GenericCurlConfig(use_curl_cabundle=True)
> >> +
> >> + curl = FakeCurl(RapiMock())
> >> + cl = client.GanetiRapiClient("master.foo.com", curl_config_fn=cfgfn,
> >> + curl=curl)
> >
> > s/foo.com/example.com/ ?
>
> Done.
>
> >> + self.assert_(curl.getopt(pycurl.SSL_VERIFYPEER))
> >> + self.assertFalse(curl.getopt(pycurl.CAINFO))
> >> + self.assertFalse(curl.getopt(pycurl.CAPATH))
> >> +
> >> + def testCertVerifyCafile(self):
> >> + mycert = "/tmp/some/cert/file.pem"
> >
> > maybe adding a comment that these paths (here and below) are not actually
> > used would be
> > helpful :)
>
> No comment, but changed the paths to include /UNUSED/.
Thanks,
LGTM then (I also rely on the fact Guido reviewed this too :)
iustin
