[PATCH] Another modified memory reference provided by value / Minor cleanup
Jeremy Allison
jra at samba.org
Fri Feb 23 23:13:16 UTC 2018
On Tue, Feb 06, 2018 at 11:25:52AM +0100, Swen Schillig via samba-technical wrote:
> Again
>
> Review appreciated.
> From df745aaa2f8efa54ca72b8baae4cd091d68f5bfc Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at vnet.ibm.com>
> Date: Tue, 6 Feb 2018 11:01:17 +0100
> Subject: [PATCH 1/2] Another modified memory reference provided by value
>
> Like the previous patch for aio.c talloc_move cannot be used
> on a memory reference which isprovided by value.
> Here as well, the caller is de-referencing the memory afterwards.
>
> Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
> ---
> source3/smbd/blocking.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c
> index 1e3a596d11f..3186dd9843f 100644
> --- a/source3/smbd/blocking.c
> +++ b/source3/smbd/blocking.c
> @@ -236,7 +236,7 @@ bool push_blocking_lock_request( struct byte_range_lock *br_lck,
> }
>
> SMB_PERFCOUNT_DEFER_OP(&req->pcd, &req->pcd);
> - blr->req = talloc_move(blr, &req);
> + blr->req = talloc_memdup(blr, req, sizeof(struct smb_request));
This one is just wrong I think. talloc_move() is the correct
call here.
> 2.14.3
>
>
> From f00c7c0a7ff3a529db489093fdb1d5b1f31d4b8f Mon Sep 17 00:00:00 2001
> From: Swen Schillig <swen at vnet.ibm.com>
> Date: Tue, 6 Feb 2018 11:19:41 +0100
> Subject: [PATCH 2/2] Minor cleanup
>
> Use talloc_zero instead individual attribute zero'ing
> and re-order code preventing a memory-allocation/deallocation
> in case of an error.
This one might be OK, however you're doing a boatload
of reformatting of code mixed in with the change.
That makes it impossible to review cleanly (plus
the reformatting you're doing on things like the
argument list to push_blocking_lock_request()
makes the code *much* harder to read, not easier).
There's a reason we do:
RETURN function(parameter1,
parameter2,
parameter3,
..
parameterN);
It's easier to read. And not:
RETURN functions(parameter1, parameter2, parameter3,
.. parameterN);
Can you separate out the single change you're doing
here, and re-post ?
Thanks,
Jeremy.
> Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
> ---
> source3/smbd/blocking.c | 89 ++++++++++++++++++-------------------------------
> 1 file changed, 32 insertions(+), 57 deletions(-)
>
> diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c
> index 3186dd9843f..e586bfb1c80 100644
> --- a/source3/smbd/blocking.c
> +++ b/source3/smbd/blocking.c
> @@ -150,38 +150,41 @@ static bool recalc_brl_timeout(struct smbd_server_connection *sconn)
> Function to push a blocking lock request onto the lock queue.
> ****************************************************************************/
>
> -bool push_blocking_lock_request( struct byte_range_lock *br_lck,
> - struct smb_request *req,
> - files_struct *fsp,
> - int lock_timeout,
> - int lock_num,
> - uint64_t smblctx,
> - enum brl_type lock_type,
> - enum brl_flavour lock_flav,
> - uint64_t offset,
> - uint64_t count,
> - uint64_t blocking_smblctx)
> +bool push_blocking_lock_request(struct byte_range_lock *br_lck,
> + struct smb_request *req, files_struct *fsp,
> + int lock_timeout, int lock_num,
> + uint64_t smblctx, enum brl_type lock_type,
> + enum brl_flavour lock_flav, uint64_t offset,
> + uint64_t count, uint64_t blocking_smblctx)
> {
> struct smbd_server_connection *sconn = req->sconn;
> - struct blocking_lock_record *blr;
> + struct blocking_lock_record *blr = NULL;
> NTSTATUS status;
>
> if (req->smb2req) {
> - return push_blocking_lock_request_smb2(br_lck,
> - req,
> - fsp,
> - lock_timeout,
> - lock_num,
> - smblctx,
> - lock_type,
> - lock_flav,
> - offset,
> - count,
> - blocking_smblctx);
> + return push_blocking_lock_request_smb2(br_lck, req, fsp,
> + lock_timeout, lock_num,
> + smblctx, lock_type,
> + lock_flav, offset, count,
> + blocking_smblctx);
> }
>
> if(req_is_in_chain(req)) {
> - DEBUG(0,("push_blocking_lock_request: cannot queue a chained request (currently).\n"));
> + DEBUG(0,("push_blocking_lock_request: cannot queue a chained "
> + "request (currently).\n"));
> + return False;
> + }
> +
> + /* Add a pending lock record for this. */
> + status = brl_lock(req->sconn->msg_ctx, br_lck, smblctx,
> + messaging_server_id(req->sconn->msg_ctx), offset,
> + count, lock_type == READ_LOCK ? PENDING_READ_LOCK :
> + PENDING_WRITE_LOCK,
> + lock_flav, True, NULL);
> +
> + if (!NT_STATUS_IS_OK(status)) {
> + DEBUG(0,("push_blocking_lock_request: failed to add "
> + "PENDING_LOCK record.\n"));
> return False;
> }
>
> @@ -189,23 +192,13 @@ bool push_blocking_lock_request( struct byte_range_lock *br_lck,
> * Now queue an entry on the blocking lock queue. We setup
> * the expiration time here.
> */
> -
> - blr = talloc(NULL, struct blocking_lock_record);
> + blr = talloc_zero(NULL, struct blocking_lock_record);
> if (blr == NULL) {
> DEBUG(0,("push_blocking_lock_request: Malloc fail !\n" ));
> return False;
> }
>
> - blr->next = NULL;
> - blr->prev = NULL;
> -
> blr->fsp = fsp;
> - if (lock_timeout == -1) {
> - blr->expire_time.tv_sec = 0;
> - blr->expire_time.tv_usec = 0; /* Never expire. */
> - } else {
> - blr->expire_time = timeval_current_ofs_msec(lock_timeout);
> - }
> blr->lock_num = lock_num;
> blr->smblctx = smblctx;
> blr->blocking_smblctx = blocking_smblctx;
> @@ -213,26 +206,8 @@ bool push_blocking_lock_request( struct byte_range_lock *br_lck,
> blr->lock_type = lock_type;
> blr->offset = offset;
> blr->count = count;
> -
> - /* Specific brl_lock() implementations can fill this in. */
> - blr->blr_private = NULL;
> -
> - /* Add a pending lock record for this. */
> - status = brl_lock(req->sconn->msg_ctx,
> - br_lck,
> - smblctx,
> - messaging_server_id(req->sconn->msg_ctx),
> - offset,
> - count,
> - lock_type == READ_LOCK ? PENDING_READ_LOCK : PENDING_WRITE_LOCK,
> - blr->lock_flav,
> - True,
> - NULL);
> -
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(0,("push_blocking_lock_request: failed to add PENDING_LOCK record.\n"));
> - TALLOC_FREE(blr);
> - return False;
> + if (lock_timeout != -1) {
> + blr->expire_time = timeval_current_ofs_msec(lock_timeout);
> }
>
> SMB_PERFCOUNT_DEFER_OP(&req->pcd, &req->pcd);
> @@ -243,8 +218,8 @@ bool push_blocking_lock_request( struct byte_range_lock *br_lck,
>
> /* Ensure we'll receive messages when this is unlocked. */
> if (!sconn->smb1.locks.blocking_lock_unlock_state) {
> - messaging_register(sconn->msg_ctx, sconn,
> - MSG_SMB_UNLOCK, received_unlock_msg);
> + messaging_register(sconn->msg_ctx, sconn, MSG_SMB_UNLOCK,
> + received_unlock_msg);
> sconn->smb1.locks.blocking_lock_unlock_state = true;
> }
>
> --
> 2.14.3
>
More information about the samba-technical
mailing list