Danek Duvall wrote:
On Tue, Feb 10, 2009 at 01:03:43AM -0600, Shawn Walker wrote:
http://cr.opensolaris.org/~swalker/pkg-2691/repo-depot-diff.txt
I've removed any thing I've addressed from your reply.
- line 122: why is this a problem?
It causes the process to hang if pkg.size isn't provided. Call this "slap
the hand of people trying to break my stuff on purpose."
How does it hang? Reading from /dev/null should return EOF immediately.
I've used this quite a bit to put dummy test files into dummy packages.
It hangs when attempting to calculate the hash in chunks. That's
because the length of the read data from /dev/null is always 0:
fhash = sha.new()
while length > 0:
data = f.read(min(bufsz, length))
fhash.update(data)
length -= len(data)
f.close()
I can't just read until EOF as that will fail if the underlying file
object is a request rfile (socket).
- line 155, 167: these clauses can be pretty bad on the memory usage.
Sorry, I'm dense at the moment. Explain a bit more?
You're reading the entire file into memory, which is a technique you've
explicitly gotten rid of in other places, for the better. So why do we do
it here?
If the file is a request rfile object (a tcp socket?) or other file-like
object, the data has to be either read completely into memory or written
to a temporary file. Lacking an alternative I could be confident was
the right choice, I left it as is. That's why I went through all the
checks for seek, etc. to make that an option of last resort.
Where do we end up using them? It might help to do something like
The only time this gets hit at the moment is during publishing to a remote
server. The receiving server, when it rebuilds the action, hands off a
request rfile object to the action class which only supports read(). I was
trying to be efficient and allow objects that support seek() and tell() to
get their size calculated in a better way than sucking in all the data from
a file.
That's fine. I'd been laboring under the mistaken impression that some
streams could only seek forwards, but apparently I'm wrong, so the
seek/tell bit is fine.
It would seem that's very possible. However, none of the streams we
currently process have that characteristic as far as I can tell.
Technically, I think I only need lines 164-166 if the object supports
"read" and I could dump the rest for our current needs.
<shrug> What's there is fine. If we run into more memory issues, we could
always write the data to a temporary file in chunks.
Given my responses above; does a temporary file seem appropriate now?
If so, what should I do about finding a temporary directory to use given
that /tmp is swap-backed?
Why do I need SEEK_SET again?
So you're not using magic constants like 2.
Was just following "pydoc file". However, I've changed this now
(everywhere we use it).
You could rewrite the size calculation in generic.py as (roughly):
So changed; sorry for being dense.
server/transaction.py:
...
- line 212: previously, we had a bunch of stub code here that would deal
with package completeness. Shouldn't that stay?
Why? I'm not a big fan of stub code; especially stub code that was getting
called and doing nothing. If you'd like, I'd be happy to open a bug with a
summary of the stubs I removed.
A bug would be fine. I just don't want to lose the requirement to do this.
Stephen probably won't forget, but I think that the rest of us probably
need a reminder stashed somewhere. I think it's less likely to get lost in
the code, but a bug would probably be okay instead.
I've opened bug 6520 for this.
server/repository.py:
- line 96: should this have a line break in here somewhere?
My opinion was that I don't know what client is using my messages, so I
should be using a minimal amount of formatting and relying on the client to
format messages as they see fit (think of the GUI wrapping the text to fit
its box according to font size for visually impaired users).
Okay. I keep dreaming about a widget that will display text in the
terminal without having to have explicit linebreaks. Kind of like
textwrap, but probably a little richer.
I realise this makes things a little less pretty for the cli case, but
given that many users will be using the gui, I thought it best to fit
that first. We should be able to add a line formatter to msg() and
emsg() to help the cli out.
I've opened bug 6519 for that.
I'm positive that the search related stuff should stay here as that is a
set of functions that operates on repository data and I intended consumers
of the repository API to have access to it.
I suppose I could be convinced.
On line 333, is the "_0" intentional?
I've already removed that; that was a leftover from one my early drafts.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
