[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Unofficial GNU/Hurd release (#644657, updated patch for review)



Hi,

Alle venerdì 12 ottobre 2012, Svante Signell ha scritto:
> > Should be updated: 
> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=644657
> 
> Next package on the list: libmemcached
> 
> Attached is an updated patch for bug #644657, fix_FTBFS4Hurd.patch,
> to the new release of libmemcached:1.0.8-1, for review.

Forget libmemcached 1.0.8 and just work on the latest, which currently
is 1.0.12. Working on an old version which exists only in Debian is not
helpful, and considering upstream releases often it wouldn't need any
handling by the Debian packager(s).

>   diff -ur libmemcached-1.0.8/clients/ms_setting.c

>  static void ms_no_config_file()
>  {
> -  char userpath[PATH_MAX];
> +  char *userpath = NULL;
> +  size_t len;
>    struct passwd *usr= NULL;
>    FILE *fd;
>  
>    usr= getpwuid(getuid());
>  
> -  snprintf(userpath, PATH_MAX, "%s/%s", usr->pw_dir, DEFAULT_CONFIG_NAME);
> +  len= strlen(usr->pw_dir) + 1 + strlen(DEFAULT_CONFIG_NAME) + 1;
> +  if ((userpath= malloc(len)) == NULL)

Accoding to the rest of the code, the malloc should be called in an own
line, and only checked in the if.

> +    exit(1);

Before, you should also print a message to stderr.

> diff -ur libmemcached-1.0.8/configure.ac
>  AC_CHECK_FUNCS([strtoull])
> +AC_CHECK_FUNCS([getcwd])

This seems pointless, given the code already uses getcwd in a couple of
places at least (including where you patched).

> diff -ur libmemcached-1.0.8/libtest/server.cc
> -        char buf[PATH_MAX];
> -        char *getcwd_buf= getcwd(buf, sizeof(buf));
> +        char *getcwd_buf= getcwd(NULL, 0);

This leaks getcwd_buf.

> diff -ur libmemcached-1.0.8/libtest/timer.hpp
> -#ifdef __MACH__
> +#if defined(__MACH__) && !defined(__GNU__) 

(and for the other similar blocks)
No, since this is specific to Mac OSX, then either you check for
__MACH__ and __APPLE__, or just __APPLE__.

Also, notes for memcached.c (which should not matter, since it does not
seem to exist in 1.0.12):

> diff -ur libmemcached-1.0.8/memcached/memcached.c
> -            snprintf(temp_portnumber_filename,
> -                     sizeof(temp_portnumber_filename),
> +            len = strlen(portnumber_filename) + 4 + 1;
> +            if ((temp_portnumber_filename = malloc(len)) == NULL) {

Same as above, most of the code has malloc/realloc in own lines.

> +                 fprintf(stderr, "Failed to allocate memory\n");
> +                 exit(EXIT_FAILURE);
> +    }

Wrong indentation for this closing bracket.

Also, did you run the tests (make check)? With both versions there is a
failure and a stuck test in libtest/unittest, and a failed test
application_doesnotexist_BINARY. On linux/x86_64, the tests of 1.0.8
later stuck, while the tests of 1.0.12 all pass.

-- 
Pino Toscano

Attachment: signature.asc
Description: This is a digitally signed message part.


Reply to: