[PATCH v2 2/3] xwayland: Remove a useless out-of-memory check

[Emil Velikov]

On 15 July 2015 at 19:11, Bryce Harrington <bryce at osg.samsung.com> wrote:
> On Wed, Jul 15, 2015 at 01:09:03PM +0100, Emil Velikov wrote:
>> Hello gents,
>>
>> On 15 July 2015 at 09:51, Marek Chalupa <mchqwerty at gmail.com> wrote:
>> > Reviewed-by: Marek Chalupa <mchqwerty at gmail.com>
>> >
>> > (http://lists.freedesktop.org/archives/wayland-devel/2015-May/021952.html)
>> >
>> > On 05/16/2015 07:38 AM, Dima Ryazanov wrote:
>> >>
>> >> snprintf does not allocate memory, so we can never get an out-of-memory
>> >> error.
>> >>
>> >> (Also, the error handler would free xwl_output after it was already
>> >> registered
>> >> as an event listener.)
>> >>
>> >> Signed-off-by: Dima Ryazanov <dima at gmail.com>
>> >> ---
>> >>   hw/xwayland/xwayland-output.c | 6 +-----
>> >>   1 file changed, 1 insertion(+), 5 deletions(-)
>> >>
>> >> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
>> >> index 9baf4eb..41937b8 100644
>> >> --- a/hw/xwayland/xwayland-output.c
>> >> +++ b/hw/xwayland/xwayland-output.c
>> >> @@ -165,11 +165,7 @@ xwl_output_create(struct xwl_screen *xwl_screen,
>> >> uint32_t id)
>> >>                                             &wl_output_interface, 2);
>> >>       wl_output_add_listener(xwl_output->output, &output_listener,
>> >> xwl_output);
>> >>
>> >> -    if (snprintf(name, sizeof name, "XWAYLAND%d", serial++) < 0) {
>> >> -        ErrorF("create_output ENOMEM\n");
>> >> -        free(xwl_output);
>> >> -        return NULL;
>> >> -    }
>> >> +    snprintf(name, sizeof name, "XWAYLAND%d", serial++);
>> >>
>> man snprintf does mention
>>
>> "      If an output error is encountered, a negative value is returned."
>>
>> It does not explicitly state under what conditions. Not sure if we
>> should really care here or not but at the very least the error message
>> is inaccurate :-)
>
> Well, snprintf errors if the size of the buffer was too small for what
> it needs to write to.  That can't happen here because the buffer is
> large (256) and the most that's going to be written is an int.
>
Obviously that'll be the case in many(all?) implementations, but this
is not guaranteed by the standard.

> So, the only way this would fail is if the code changed, which would be
> a programming error.  In which case maybe an assert() would be more
> appropriate?
>
>From a quick look only a few places in xserver check the return value
- xquarts, os and xkb. In (most of) these cases the buffer size may
not sufficient. So I think one can safely ignore my comment and don't
bother with the assert :-)

Cheers,
Emil