Hello, On Mon, 14 Nov 2011, Andrew Stormont wrote: > From: Andrew Stormont <[email protected]> > Date: Thu, 10 Nov 2011 14:03:18 +0000 > To: <[email protected]> > 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 <[email protected]> > * Copyright © 2000,2001 Wichert Akkerman <[email protected]> > + * 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/ -- To UNSUBSCRIBE, email to [email protected] with a subject of "unsubscribe". Trouble? Contact [email protected] Archive: http://lists.debian.org/[email protected]
