Re: FW: dpkg support for Solaris
Hello,
On Mon, 14 Nov 2011, Andrew Stormont wrote:
> From: Andrew Stormont <andrew.stormont@nexenta.com>
> Date: Thu, 10 Nov 2011 14:03:18 +0000
> To: <debian-dpkg@lists.debian.org>
> Subject: dpkg support for Solaris
>
> Hi,
>
> Please could you apply the attached patch which fixes dpkg Support for
> Solaris.
It would help if you justified the changes... I'm not familiar enough with
Solaris/Nexenta to be able to judge the patch. So I would like some sort
of rationale... (if you sent proper commits generated with git
format-patch that would have been taken care of — provided that you write
good commit logs)
Let's skim through the patch for a quick review. Note however that I'm not
familiar with portability issues and some of my remarks might be
ill-advised. Feel free to point it out if you notice something odd.
> diff --git a/dselect/main.cc b/dselect/main.cc
> index 6508b1f..5b1c08b 100644
> --- a/dselect/main.cc
> +++ b/dselect/main.cc
> @@ -4,6 +4,7 @@
> *
> * Copyright © 1994,1995 Ian Jackson <ian@chiark.greenend.org.uk>
> * Copyright © 2000,2001 Wichert Akkerman <wakkerma@debian.org>
> + * Copyright © 2011 Nexenta Systems Inc. All rights reserved.
Please drop those "All rights reserved". It doesn't mix well with the
definition of the GPL...
> @@ -45,6 +46,7 @@
> #elif defined(HAVE_NCURSES_TERM_H)
> #include <ncurses/term.h>
> #else
> +#include <curses.h>
> #include <term.h>
> #endif
What does curses.h define that was missing?
> @@ -53,6 +55,10 @@
> #include <dpkg/dpkg-db.h>
> #include <dpkg/options.h>
>
> +#ifndef ERR
> +#define ERR (-1)
> +#endif
> +
> #include "dselect.h"
> #include "bindings.h"
> #include "pkglist.h"
On my Linux system ERR is defined by <curses.h>. Why is that needed on
Solaris when you already added curses.h?
> diff --git a/lib/dpkg/md5.c b/lib/dpkg/md5.c
> index 3da18c9..5e9f311 100644
> --- a/lib/dpkg/md5.c
> +++ b/lib/dpkg/md5.c
> @@ -15,6 +15,8 @@
> * MD5Context structure, pass it to MD5Init, call MD5Update as
> * needed on buffers full of bytes, and then call MD5Final, which
> * will fill a supplied 16-byte array with the digest.
> + *
> + * Copyright © 2011 Nexenta Systems Inc. All rights reserved.
> */
That file is in the public domain and it's best if we keep it that way, so
please accept the same and don't claim any copyright on it.
> @@ -41,7 +43,7 @@
> (cp)[1] = (value) >> 8; \
> (cp)[0] = (value); } while (0)
>
> -static u_int8_t PADDING[MD5_BLOCK_LENGTH] = {
> +static uint8_t PADDING[MD5_BLOCK_LENGTH] = {
> 0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
Hum, C99 is not a requirement to build dpkg. Some features are required
but those standard types are currently not part of it (see README and
doc/coding-style.txt). So maybe it's better to add the required typedefs
specifically for Solaris?
That said I don't really know why Guillem did not mandate C99 in its
entirety.
> diff --git a/lib/dpkg/md5.h b/lib/dpkg/md5.h
> index 731a2af..5f3a869 100644
> --- a/lib/dpkg/md5.h
> +++ b/lib/dpkg/md5.h
> @@ -10,27 +10,33 @@
> * This code has been tested against that, and is equivalent,
> * except that you don't need to include two pages of legalese
> * with every copy.
> + *
> + * Copyright © 2011 Nexenta Systems Inc. All rights reserved.
> */
>
> #ifndef _MD5_H_
> #define _MD5_H_
>
> +#include <config.h>
> +
An header file should not include <config.h> (even if that particular
header file is not a public one).
> +#ifdef HAVE_SYS_CDEFS
> #include <sys/cdefs.h>
> +#endif
So this test should probably be changed into something else. Not sure
what though... this header is provided by glibc but is not glibc specific
apparently.
If we can't find anything better, we could go with this I guess:
#if !defined(__sun)
#include <sys/cdefs.h>
#endif
> @@ -31,6 +33,7 @@
> # define OSHurd
> #elif defined(__sun)
> # define OSsunos
> +# undef HAVE_KVM_H
> #elif defined(OPENBSD) || defined(__OpenBSD__)
> # define OSOpenBSD
> #elif defined(hpux)
Why? Does kvm.h exist on Solaris and is it something totally unrelated?
> +#if defined(OSsunos)
> + sprintf(buf, "/proc/%d/status", pid);
> +#else
> sprintf(buf, "/proc/%d/stat", pid);
> +#endif
Does /proc/<pid>/status on Solaris follow the same structure than the
/proc/<pid>/stat on Linux, that is having the executable name
between parenthesis somewhere in the string?
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Pre-order a copy of the Debian Administrator's Handbook and help
liberate it: http://debian-handbook.info/go/ulule-rh/
Reply to: