pidgin-devel

[Gaim-devel] Oscar FT update

[William T. M. <wt...@du...>]

Hi,

This patch makes a few minor changes to Oscar file transfer.  They
should all be uncontroversial; please apply.

* Add some comments I inadvertently left out of my last patch.
* Fix a double-free that occurs when connections time out.
* Correct a bug causing filenames to be truncated by 4 characters on
  some clients.
* Preserve directory structure when sending multiple files.
* Handle (throw away) resource forks sent by Mac clients.

BTW, there is still stuff to be done for file transfer--I'll try to
work on them as time permits, but here's my list for now:
* Reverse connections for receiving from behind a firewall (reverse
connections for sending should work)
* Connections through ars.aol.oscar.com:5190 when both sides are
behind a firewall (I have the protocol for this)
* Support non-ASCII filenames--it seems they are sent over the network
as UCS-2 so we need to convert to/from UTF-8
* Either implement getfile support or rip out all the obsolete code in
ft.c (I'm leaning toward the latter :)
* Rewrite the generic FT code--I think the interface is OK, but my
current implementation has some race conditions.  To avoid conflicts I
don't plan to touch this until the FT GUI is finished.

-- Wil

Re: [Gaim-devel] Oscar FT update

[Christian H. <ch...@gn...>]

Off topic, but..

Got a question about your file transfer interface. I'm working on file
transfer for MSN, and MSN sends out little headers before each block
of data. The header is three bytes, and the 2nd and 3rd byte make up
the length of the data. So, the header is supposed to be read in and
then that number of bytes is supposed to be read in.

I'm not quite sure how to do this with the current API. It seems I can
setup a function that it will keep calling to do something with the
data, but I don't want to manually store the data in a buffer and
parse it myself. Maybe what I want can be done with the current API,
but I don't see how...

Basically, I need to be able to read in x amount of bytes, then say,
"Okay, you get 'y' many bytes and stick it in the file, and then call
me back when you're done and we'll continue this after I rest a bit."

Make sense? I hope so, because I'm not repeating it!

.. Unless it didn't make sense, in which case I'll say it again, only
differently.

Oh, and while I'm here... the fprintf and fscanf calls in ft_callback
seem... unnecessary. I'm tired, so please forgive any errors that may
cause an endless loop of kernel panics, but wouldn't these be better?

write(xfer->file, buf, rt);

and

read(xfer->file, buf, remain);

That would cut out the loops and the overly (for this case) expensive
calls to fprintf and fscanf.

And I want a puppy.

Christian

-- 
Christian Hammond         <>  The GNUpdate Project
ch...@gn...      <>  http://www.gnupdate.org/
   "My brothers and sisters all hated me cause I was an only child."
      -- Weird Al Yankovich

Re: [Gaim-devel] Oscar FT update

[William T. M. <wt...@du...>]

On Sun, Nov 10, 2002 at 03:14:37AM -0800, Christian Hammond wrote:
>
> Basically, I need to be able to read in x amount of bytes, then say,
> "Okay, you get 'y' many bytes and stick it in the file, and then call
> me back when you're done and we'll continue this after I rest a bit."

You're right, there's no easy way to do that using the current API.  I
was hoping that no sane protocol would break up a file and send
headers in the middle, since it's not necessary over TCP.  Apparently
that was a bad assumption. :-(

It shouldn't be too hard to allow what you want (I'm thinking just add
another parameter to transfer_in_do() specifying the number of bytes
to _actually_ read).  But yes, an API change is necessary if this is
what you need.

> Oh, and while I'm here... the fprintf and fscanf calls in ft_callback
> seem... unnecessary. I'm tired, so please forgive any errors that may
> cause an endless loop of kernel panics, but wouldn't these be better?
> 
> read(xfer->file, buf, remain);

Yes, I'm aware that this would be much better than calling fscanf for
every byte, but as I mentioned I'm afraid making major changes to ft.c
would conflict with the GUI work.  Unless Sean says this wouldn't be a
problem?

-- Wil

Re: [Gaim-devel] Oscar FT update

[Ethan B. <ebl...@cs...>]

William T. Mahan spake unto us the following wisdom:
> On Sun, Nov 10, 2002 at 03:14:37AM -0800, Christian Hammond wrote:
> >
> > Basically, I need to be able to read in x amount of bytes, then say,
> > "Okay, you get 'y' many bytes and stick it in the file, and then call
> > me back when you're done and we'll continue this after I rest a bit."
>=20
> You're right, there's no easy way to do that using the current API.  I
> was hoping that no sane protocol would break up a file and send
> headers in the middle, since it's not necessary over TCP.  Apparently
> that was a bad assumption. :-(

=2E.. But what if you want to interleave FT and other info on the same
stream?  I can see this as being a very reasonable thing to do.

Ethan

--=20
And if I claim to be a wise man / it surely means that I don't know.
                -- Kansas, "Carry on Wayward Son"

Re: [Gaim-devel] Oscar FT update

[Sean E. <se...@se...>]

I share Chip's concern.  When I first sat down to write the file
transfer GUI was the first time I looked at the API and I noticed this
right away.  Wouldn't it be better for the prpl to handle its sockets
itself and have its own callback which can strip headers and stuff and
then call a generic function with a pointer and a length?  That would
seem to be the optimal solution.

Apart from writing gtkcellrendererprogress, I've not started the file
transfer GUI--Rob may have started--either way changes to ft.c would not
interfere with it much, I'm sure.

-s.

On Sun, 2002-11-10 at 14:08, Ethan Blanton wrote:
> William T. Mahan spake unto us the following wisdom:
> > On Sun, Nov 10, 2002 at 03:14:37AM -0800, Christian Hammond wrote:
> > >
> > > Basically, I need to be able to read in x amount of bytes, then say,
> > > "Okay, you get 'y' many bytes and stick it in the file, and then call
> > > me back when you're done and we'll continue this after I rest a bit."
> > 
> > You're right, there's no easy way to do that using the current API.  I
> > was hoping that no sane protocol would break up a file and send
> > headers in the middle, since it's not necessary over TCP.  Apparently
> > that was a bad assumption. :-(
> 
> ... But what if you want to interleave FT and other info on the same
> stream?  I can see this as being a very reasonable thing to do.
> 
> Ethan
> 
> -- 
> And if I claim to be a wise man / it surely means that I don't know.
>                 -- Kansas, "Carry on Wayward Son"

Re: [Gaim-devel] Oscar FT update

[William T. M. <wt...@du...>]

On Sun, Nov 10, 2002 at 02:23:47PM -0500, Sean Egan wrote:
> I share Chip's concern.  When I first sat down to write the file
> transfer GUI was the first time I looked at the API and I noticed this
> right away.  Wouldn't it be better for the prpl to handle its sockets
> itself and have its own callback which can strip headers and stuff and
> then call a generic function with a pointer and a length?  That would
> seem to be the optimal solution.

I agree that this seems like a good generic solution, given the
situation Chip described.  This wasn't really an oversight in the API,
but rather an intentional assumption.  Although I admit it was a poor
one, as Paco and others have pointed out.

The main drawback to your method is that there will be some
duplication of code in each of prpls (the read/write while loops).  I
supppose it's worth it to have a simpler interface, though.  I'll work
on implementing something like what you suggest.

> Apart from writing gtkcellrendererprogress, I've not started the file
> transfer GUI--Rob may have started--either way changes to ft.c would not
> interfere with it much, I'm sure.

OK, I'll focus on these changes to the generic interface then, since
it seems to be most in need of work.

-- Wil
P.S.  Not to nag, but I hope someone remembers to apply my patch that
started this thread. :-)

Re: [Gaim-devel] Oscar FT update

[William T. M. <wt...@du...>]

On Sun, Nov 10, 2002 at 03:39:16PM -0500, Ethan Blanton wrote:
>
> I'm not sure precisely what read/write while loops you're speaking of,
> but we *really* need to avoid I/O requiring synchronicity (yes, that's

What I meant is that the I/O handlers that actually call read() and
write() on sockets will need to be moved out of the generic ft.c and
into the prpls.  This means more work for the prpls but a simpler
interface.

> a word) wherever possible.  If a read or write fails to complete 100%
> (i.e. you wanted a 2k packet and you only got 1k of it), you need to
> mark this down someplace and hand control back to gaim until your
> socket comes up as readable again.  forcing synchronicity is where we
> get the lags we have in the UI ... most (if not all) of our sockets
> are actually nonblocking, we just force blocking behavior in places.

I agree with your point in general, although I think if you look at
the FT code in particular you will find that it is nicely
asynchronous. :)

> This may mean that prpls need to keep track of more state on a
> per-connection or per-socket basis, but it's worth the effort...

OK, here's my shot at a new interface.  What follows is the part
visible to the prpls (i.e. this goes in prpl.h).

This is somewhat different from the current API, but I want to get it
right this time; comments are welcome. 

======

/* A struct, opaque to the prpls, that uniquely identifies
 * a file transfer session (a session consists of sending
 * one or more files to a given user).
 */
struct file_transfer;


/* Functions called by the prpls */
/* ----------------------------- */

/* Set up an incoming session; this will prompt the user whether to
 * accept the transfer, where to put the files, etc.
 */
extern struct file_transfer *file_transfer_in_add(struct gaim_connection *gc,
		const char *who, const char *name, long totsize,
		long totfiles, const char *msg);

/* Set up an outgoing session; this will prompt the user for which
 * file(s) to send.
 */
extern struct file_transfer *file_transfer_out_add(struct gaim_connection *gc,
		const char *who);

/* For outgoing transfers, this is called before sending each file
 * to get the filename and size; and to set the offset for resuming.
 * For incoming transfers, this is called before receiving each file
 * to set the filename and size; and to get the offset for resuming.
 * Also opens the file for reading/writing as appropriate.
 */
extern int file_transfer_start_file(struct file_transfer *xfer,
	char **filename, long *size, long *offset);

/* Read or write a chunk of data.  For incoming transfers,
 * reads from buf and writes to a file; for outgoing transfers,
 * reads from a file and writes to buf.
 */
extern int file_transfer_do_chunk(struct file_transfer *xfer,
	char *buf, int len);

/* This is used to get information about a session.
 */
extern int file_transfer_get_info(struct file_transfer *xfer,
	char **name, long *totfiles, long *totsize);

/* Abort a session in progress. */
extern int file_transfer_abort(struct file_transfer *xfer, const char *why);


/* Callbacks that each prpl should provide */
/* --------------------------------------- */

struct prpl {
	/* ... */

	/* A session was successfully set up; the prpl may begin sending
	 * or receiving the first file.
	 */
	void (* file_transfer_added)      (struct gaim_connection *gc, struct file_transfer *xfer);
	/* A session was canceled by the user; the prpl should clean up. */
	void (* file_transfer_canceled)   (struct gaim_connection *gc, struct file_transfer *xfer);

};

-- Wil

Re: [Gaim-devel] Oscar FT update

[Christian H. <ch...@gn...>]

On Sun, Nov 10, 2002 at 09:16:58PM -0500, William T. Mahan wrote:
> On Sun, Nov 10, 2002 at 03:39:16PM -0500, Ethan Blanton wrote:
> >
> > I'm not sure precisely what read/write while loops you're speaking of,
> > but we *really* need to avoid I/O requiring synchronicity (yes, that's
> 
> What I meant is that the I/O handlers that actually call read() and
> write() on sockets will need to be moved out of the generic ft.c and
> into the prpls.  This means more work for the prpls but a simpler
> interface.

There's a simple solution for this. You just add two new functions to
the prpl interface. One for reading, one for writing. Before doing a
read() or write() (you should be using these instead of the loops
ANYWAY, or at least a less costly function, like fgetc or fputc) you
should check if these functions are NULL. If they're NULL, use
read/write. If not, use the functions.

> OK, here's my shot at a new interface.  What follows is the part
> visible to the prpls (i.e. this goes in prpl.h).

I haven't had a chance to read it yet. I'll look at it in a sec.

> This is somewhat different from the current API, but I want to get it
> right this time; comments are welcome. 

You'll probably get them soon :) My friend, Steph, has pics for me. I
want them. This is why MSN is about to have file transfer, but that
won't be done until the FT API works with it... so I have some
incentive at the moment ;)

Christian

-- 
Christian Hammond         <>  The GNUpdate Project
ch...@gn...      <>  http://www.gnupdate.org/
   Why doesn't the Bat Computer ever crash?

Re: [Gaim-devel] Oscar FT update

[Marcel B. <ma...@ca...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


| You'll probably get them soon :) My friend, Steph, has pics for me. I
| want them.
	...
| Christian
|

Chip gonna get some cyber-booty.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9z3s3RP2ZrlyJqb0RAggfAKC0XdCxITNRRooIfCf7q8yXdjahnACgoKrW
S+vfvyaULJSN9WEKcDnmvUw=
=YwKY
-----END PGP SIGNATURE-----

Re: [Gaim-devel] Oscar FT update

[Christian H. <ch...@gn...>]

On Mon, Nov 11, 2002 at 01:41:11AM -0800, Marcel Birthelmer wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> | You'll probably get them soon :) My friend, Steph, has pics for me. I
> | want them.
> 	...
> | Christian
> |
> 
> Chip gonna get some cyber-booty.

Actually, no.. we're just friends...

Christian

-- 
Christian Hammond         <>  The GNUpdate Project
ch...@gn...      <>  http://www.gnupdate.org/
   What is it, Lassie?  A boy fell down a mine shaft and broke his ankle
   and is diabetic and needs insulin?  Is THAT what you're trying to tell me?

Re: [Gaim-devel] Oscar FT update

[Christian H. <ch...@gn...>]

On Sun, Nov 10, 2002 at 02:23:47PM -0500, Sean Egan wrote:
> I share Chip's concern.  When I first sat down to write the file
> transfer GUI was the first time I looked at the API and I noticed this
> right away.  Wouldn't it be better for the prpl to handle its sockets
> itself and have its own callback which can strip headers and stuff and
> then call a generic function with a pointer and a length?  That would
> seem to be the optimal solution.
> 
> Apart from writing gtkcellrendererprogress, I've not started the file
> transfer GUI--Rob may have started--either way changes to ft.c would not
> interfere with it much, I'm sure.
> 
> -s.

Funny how I just got this post. I *thought* I was missing something in
the reply!

The simple addition of read and write functions, defaulting to libc
read() and write() if they're NULL, allowed for file transfer in MSN.
It works beautifully, and I've received many documents and images
so far. Now I just want that file transfer dialog! ;)

Christian

-- 
Christian Hammond         <>  The GNUpdate Project
ch...@gn...      <>  http://www.gnupdate.org/
   96.7% of all statistics are made up.