Return-Path: Date: Fri, 13 Jan 2012 13:21:11 +0200 From: Johan Hedberg To: Hendrik Sattler Cc: Jaganath Kanakkassery , linux-bluetooth@vger.kernel.org Subject: Re: [PATCH obexd 2/2] gobex: Fix ABORT request not processing Message-ID: <20120113112111.GA5880@x220> References: <1326448613-8367-1-git-send-email-jaganath.k@samsung.com> <20120113105420.GA4490@x220> <8d043367426e37a570df29ff2f4b298b@mail.hendrik-sattler.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <8d043367426e37a570df29ff2f4b298b@mail.hendrik-sattler.de> Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Hendrik, On Fri, Jan 13, 2012, Hendrik Sattler wrote: >>>> opcode = obex->rx_last_op; >>>> /* Unexpected response -- fail silently */ >>>>- if (opcode > 0x1f && opcode < 0xff) { >>>>+ if (opcode > G_OBEX_OP_ABORT && opcode < 0xff) { >>>> obex->rx_data = 0; >>>> return TRUE; >>>> } >>> >>>This one always evaluates to false because 0xff == G_OBEX_OP_ABORT | >>>FINAL_BIT. >> >> The value of obex->rx_last_op is stored with the final bit cleared, >> so the < 0xff part could also be removed from the test. The value >> 0x1f has been picked because the IrDA OBEX specification defines 0x10 >> - 0x1f as a user-definable range, so anything between 0x1f and 0x7f >> isn't actually valid. The correct test would then become: >> >> if (opcode > 0x1f && opcode != G_OBEX_OP_ABORT) >> >> Jaganath, with the above change the patch should be ok, but the more >> surprising thing to me here is that this implies we're missing one or >> more unit tests for Abort. Could you create a patch to add those too? >> (if we're really strict those tests should go in before this patch so >> that it's actually possible to see that the patch makes a >> difference). > > User-definable does not mean invalid. It only means that these can be > used for custom commands. These are still bound to the rules of the > OBEX protocol. I've never seen one using that, though. Exactly. You might want to re-read what I said and the test I proposed. > Else you'd also have to filter 0x04 and the range 0x08-0x0f because > these are marked as "reserved". With the test I proposed we filter neither reserved (since they might get meaning in new spec versions) nor user-defined opcodes. Johan