Created attachment 78892 [details] [review]
application patch

the patch is a combination of Nathan's (Nathan Owens <pianocomp81@yahoo.com>)patch and Solaris internal patch.Owens <pianocomp81@yahoo.com>

Committed to HEAD.  Thanks Nathan and Jerry Yu.  Let me keep the bug open to address Matt's concerns.

Matt's concerns:
> I've had a look at the patch.  I don't like the changes to the
> "local_record_to_pilot_record" API.  What happens if the record is
> longer than the array the user has supplied?  You've no way of informing
> the user that they hadn't allocated enough space, or what the right
> amount of space is.  You should get the caller to pass in a non-null
> (pi_buffer_t *).  For thread safeness it would be best not to use static
> pi_buffers_t variables.  Let the caller create a temporary one and
> destroy it when done.
> 
> Also, there are a couple of places where there's an explicit
> free(piBuf.data) that should be pi_buffer_free(piBuf).
> 
> Other than that, looks good to me.

The patch pasted here modifying local_record_to_pilot_record isn't different from Nathan's original patch. It may resolve Matt's concerns.

Thanks for committing it!

I don't think it really resolves Matt's concerns, but it's no different than before. The same thing can happen: data could be truncated if record[] isn't big enough. Now, I don't think it's a problem in this format (or the previous format) only because 0xffff is the maximum PalmOS record size right now.

Matt's real concern was that record should be a pi_buffer_t, and should be passed in the whole way, automatically resized by pack_Memo() if needed, and then returned to the user. This is not possible because the prepare() function (implements the gnome-pilot 'prepare' signal) calls local_record_to_pilot_record(). Right now, any memory allocated in prepare() must be freed in prepare(). To completely alleviate Matt's concern, prepare() would return dynamically-allocated memory, and then some future call (by gnome-pilot and pilot-link) would free that memory. This is a limitation in the pilot-link libpisync API. It doesn't look like this limitation will be overcome in the near future, either.

Given all that, I'd agree with Jerry that this bug can be closed - I've looked at SVN, and it looks good. Thanks again!

It's great to see this patch committed.

It's still the case that local_record_to_pilot_record returns the record in a static char[], so is thread unsafe.  I agree with Nathan that this isn't a big problem (it isn't a regression, AFAICS): gpilotd only runs one sync at a time, so I think you'd need multiple copies of gpilotd, one listening for, say, usb and the other for network.

That said, the obvious way to use a pi_buffer in the conduits would be to give each conduit context (e.g. struct _EAddrConduitContext) its own pi_buffer to use as required.  The memory can then be freed in e_addr_context_destroy.  local_record_to_pilot_record could use the same API as before, but return a pointer to the conduit's pi_buffer data instead of the static char[].

Hi,
I want to use this patch to get evolution compiled with the new pilot-link, but it doesn't work here, it keeps looking for the old pilot-link functions.
Do I have to pass a special parameter to configure to make it check if I have the new version of pilot-link installed?

Tom, 
Perhaps you should add this option "--with-pisock=$PILOT_LINK_PATH" when configuring.

Hmm,
why is this bug not closed? As far as I see the patch is already committed in evolution 2.10 and perfectly working

Bumping version to a stable release.