Skip to content

Stable 0.9 ClickHouse Patch#2

Closed
nikitamikhaylov wants to merge 6 commits intostable-0.9from
stable-0.9-clickhouse-patch
Closed

Stable 0.9 ClickHouse Patch#2
nikitamikhaylov wants to merge 6 commits intostable-0.9from
stable-0.9-clickhouse-patch

Conversation

@nikitamikhaylov
Copy link
Copy Markdown
Member

This is the version which is currently in use.

InfJoker and others added 6 commits August 28, 2023 14:02
(cherry picked from commit b36eeb6f3e9117c9fecad32c977bef051dc13046)
…ccept_fd

(cherry picked from commit 11289d610a055cdd6244c781347319af7ec4eab6)
(cherry picked from commit 12f8132309f96f30f5632d964e7f101c41bd5a15)
Fixes: https://gitlab.com/libssh/libssh-mirror/-/issues/15
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
(cherry picked from commit 95fb4b3993202fa1d0dbff897642b318d935e63b)
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Sep 29, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ InfJoker
✅ bkuschel
✅ alexey-milovidov
❌ Jakuje
You have signed the CLA already but the status is still pending? Let us recheck it.

Revertionist pushed a commit to Revertionist/libssh-fork that referenced this pull request Nov 13, 2025
The existing sftp async read api has two problems :

1. sftp_async_read() assumes that the value of the third
parameter count is same as the number of bytes requested
to read in the corresponding call to sftp_async_read_begin().

But the documentation of sftp_async_read() allows the value of
count parameter to be more than that requested length. If value
of count parameter is more than that requested length then
sftp_async_read() updates the file->offset incorrectly which
leads to further read/writes occuring from incorrect offsets.

The problem here is that sftp_async_read() doesn't know about
the number of bytes requested to read specified in the call to
sftp_async_read_begin(), and it wrongly assumes the value
of its count parameter (which is actually the size of the buffer
to store the read data) to be the same as the number of bytes
requested to read.

2. sftp_async_read_begin() returns an uint32_t type value type
casted to int as a request identifier, whereas sftp_async_read()
expects an uint32_t type value as a request identifier. Due to this
the user has to typecast the identifier returned by sftp_async_read_begin()
from int to uint32_t and then pass it to sftp_async_read(). This
type casting is cumbersome for the user and hence the approach is
not user-friendly.

This commit solves the above two problems by introducing a new
sftp aio api.

The sftp_aio_begin_*() functions in the api send an i/o request to
the sftp server and provide the caller a dynamically allocated
structure storing information about the sent request. Information
like number of bytes requested for i/o, id of sent request etc is
stored in the structure.

That structure should be provided to the sftp_aio_wait_*() functions
in the api which wait for the response corresponding to the request whose
info is stored in the provided structure.

The libssh user is supposed to handle that structure through an
opaque type sftp_aio.

Since the structure stores the number of bytes requested for i/o,
sftp_aio_wait_*() knows about the number of bytes requested for i/o
(specified in the call to sftp_aio_begin_*()) and hence updates the
file->offset correctly solving problem ClickHouse#1 present in the existing
async api.

Since the structure provided by sftp_aio_begin_*() (containing the
request id) is supplied to sftp_aio_wait_*(), no casting of id's
needs to be done by the user solving problem ClickHouse#2 of the existing
async api.

Signed-off-by: Eshan Kelkar <eshankelkar@galorithm.com>
Reviewed-by: Sahana Prasad <sahana@redhat.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants