Re: Extending libdebian-installer's API to include certainty on subarch detection
Here's a revised libd-i patch:
=== modified file 'configure.ac'
--- configure.ac 2008-02-25 09:49:18 +0000
+++ configure.ac 2010-09-09 05:16:00 +0000
@@ -11,7 +11,7 @@
LIBRARY_VERSION_MAJOR=4
LIBRARY_VERSION_MINOR=0
-LIBRARY_VERSION_REVISION=6
+LIBRARY_VERSION_REVISION=7
LIBRARY_VERSION="$LIBRARY_VERSION_MAJOR.$LIBRARY_VERSION_MINOR.$LIBRARY_VERSION_REVISION"
LIBRARY_VERSION_LIBTOOL="$LIBRARY_VERSION_MAJOR:$LIBRARY_VERSION_MINOR:$LIBRARY_VERSION_REVISION"
=== modified file 'include/debian-installer/system/subarch.h'
--- include/debian-installer/system/subarch.h 2005-08-03 09:42:05 +0000
+++ include/debian-installer/system/subarch.h 2010-09-09 08:34:43 +0000
@@ -24,6 +24,10 @@
#ifndef DEBIAN_INSTALLER__SYSTEM__SUBARCH_H
#define DEBIAN_INSTALLER__SYSTEM__SUBARCH_H
+#ifdef __ARMEL__
+#define DI_SYSTEM_SUBARCH_CAN_GUESS 1
+#endif
+
/**
* @addtogroup di_system_subarch
* @{
@@ -35,5 +39,13 @@
*/
const char *di_system_subarch_analyze (void);
+/**
+ * Return a string with a best-guess of the current subarchitecture
+ *
+ * Only present on armel currently, and is a stub on all other architectures
+ */
+
+const char *di_system_subarch_analyze_guess (void);
+
/** @} */
#endif
=== modified file 'src/libdebian-installer.ver'
--- src/libdebian-installer.ver 2006-09-30 15:06:52 +0000
+++ src/libdebian-installer.ver 2010-09-09 06:56:03 +0000
@@ -155,6 +155,11 @@
di_tree_size;
} LIBDI_4.5;
+LIBDI_4.7 {
+ global:
+ di_system_subarch_analyze_guess;
+} LIBDI_4.6;
+
#LIBDI_PRIVATE {
# global:
# internal_*;
=== modified file 'src/system/subarch-arm-linux.c'
--- src/system/subarch-arm-linux.c 2010-05-06 14:47:46 +0000
+++ src/system/subarch-arm-linux.c 2010-09-09 08:36:29 +0000
@@ -3,6 +3,7 @@
#include <stdio.h>
#include <string.h>
#include <strings.h>
+#include <sys/utsname.h>
#include <debian-installer/system/subarch.h>
@@ -11,6 +12,13 @@
char *ret;
};
+static const char *supported_generic_subarches[] = {
+ "dove",
+ "omap",
+ "omap4",
+ NULL
+};
+
static struct map map_hardware[] = {
{ "Acorn-RiscPC" , "rpc" },
{ "EBSA285" , "netwinder" },
@@ -102,3 +110,27 @@
return "unknown";
}
+
+const char *di_system_subarch_analyze_guess(void)
+{
+ struct utsname sysinfo;
+ size_t uname_release_len, i;
+
+ /* Attempt to determine subarch based on kernel release version */
+ uname(&sysinfo);
+ uname_release_len = strlen(sysinfo.release);
+
+ for (i = 0; supported_generic_subarches[i] != NULL; i++)
+ {
+ size_t subarch_len = strlen (supported_generic_subarches[i]);
+ if (!strncmp(sysinfo.release+uname_release_len-subarch_len,
+ supported_generic_subarches[i],
+ subarch_len))
+ {
+ return supported_generic_subarches[i];
+ }
+ }
+
+ /* If we get here, try falling back on the normal detection method */
+ return di_system_subarch_analyze();
+}
=== modified file 'src/system/utils.c'
--- src/system/utils.c 2005-07-31 16:10:46 +0000
+++ src/system/utils.c 2010-09-09 05:19:47 +0000
@@ -22,6 +22,7 @@
#include <config.h>
+#include <debian-installer/system/subarch.h>
#include <debian-installer/system/utils.h>
#include <debian-installer/log.h>
@@ -36,4 +37,16 @@
di_log_set_handler (DI_LOG_LEVEL_MASK, di_log_handler_syslog, NULL);
}
-
+#ifndef DI_SYSTEM_SUBARCH_CAN_GUESS
+
+/*
+ * HACK: If there's a better way to do this, we should probably use that
+ * instead of this stub function for non armel archs
+ */
+
+const char *di_system_subarch_analyze_guess (void)
+{
+ return di_system_subarch_analyze();
+}
+
+#endif
On Wed, Sep 08, 2010 at 09:54:56AM +0000, Colin Watson wrote:
> On Wed, Sep 08, 2010 at 01:51:20AM -0400, Michael Casadevall wrote:
> > Perfectly. I've reworked my patches to implement these suggestions, here's my
> > current result:
> >
> > === modified file 'include/debian-installer/system/subarch.h'
> > --- include/debian-installer/system/subarch.h 2005-08-03 09:42:05 +0000
> > +++ include/debian-installer/system/subarch.h 2010-09-07 08:09:28 +0000
> > @@ -24,6 +24,10 @@
> > #ifndef DEBIAN_INSTALLER__SYSTEM__SUBARCH_H
> > #define DEBIAN_INSTALLER__SYSTEM__SUBARCH_H
> >
> > +#ifdef __ARMEL__
> > +#define SUBARCH_CAN_GUESS 1
> > +#endif
>
> This is namespace pollution. libd-i's namespace isn't all *that*
> regular, but I think DI_SYSTEM_SUBARCH_CAN_GUESS would be a bit more
> conventional.
>
> > +/**
> > + * Return a string with a best-guess of the current subarchitecture
> > + *
> > + * Only present on armel currently, and is a stub on all other subarchs
>
> "all other architectures"
>
> > + */
> > +
> > +const char *di_system_subarch_analyze_guess(void);
>
> Local convention is space before parenthesis (libd-i is inconsistent,
> but we should at least be consistent within each file).
>
> > === modified file 'src/libdebian-installer.ver'
> > --- src/libdebian-installer.ver 2006-09-30 15:06:52 +0000
> > +++ src/libdebian-installer.ver 2010-09-07 08:03:43 +0000
> > @@ -146,6 +146,7 @@
> >
> > LIBDI_4.6 {
> > global:
> > + di_system_subarch_analyze_guess;
> > di_tree_destroy;
> > di_tree_foreach;
> > di_tree_insert;
>
> To add a new interface, I think we need to bump to 4.7
> (LIBRARY_VERSION_REVISION=7 in configure.ac) and add a new block in
> src/libdebian-installer.ver. This shouldn't require any
> reverse-dependencies to be rebuilt.
>
> (Otavio, I don't suppose you'd like to comment on whether this would be
> possible for squeeze?)
>
> > +const char *di_system_subarch_analyze_guess (void)
> > +{
> > + struct utsname sysinfo;
> > + int uname_release_len, i;
> > +
> > + /* Attempt to determine subarch based on kernel release version */
> > + uname(&sysinfo);
> > + uname_release_len = strlen(sysinfo.release);
> > +
> > + for (i = 0; supported_generic_subarches[i] != NULL; i++)
> > + {
> > + int subarch_len = strlen (supported_generic_subarches[i]);
> > + if (!strncmp(sysinfo.release+uname_release_len-subarch_len,
> > + supported_generic_subarches[i],
> > + subarch_len))
>
> strlen returns size_t and strncmp takes size_t as its third argument, so
> uname_release_len and subarch_len should be size_t rather than int.
>
> > +#ifndef SUBARCH_CAN_GUESS
> > +
> > +/**
> > + * HACK: If there's a better way to do this, we should probably use that
> > + * instead of thus stub function for non armel archs
> > + */
> > +
> > +const char *di_system_subarch_analyze_guess (void)
> > +{
> > + return di_system_subarch_analyze();
> > +}
> > +
> > +#endif
>
> As I think you suggested on IRC, a weak symbol would probably be neater,
> but this will do fine for now. (But s/thus/this/ in the comment, and
> that probably doesn't need to be a Doxygen comment with the leading
> "/**".)
>
> > -int main(int argc ATTRIBUTE_UNUSED, char *argv[] ATTRIBUTE_UNUSED)
> > +int main(int argc, char *argv[])
> > {
> > const char *subarch;
> >
> > + while (1)
> > + {
> > + int option_index = 0;
> > + int c;
> > +
> > + c = getopt_long (argc, argv, "g", long_options,
> > + &option_index);
> > + if (c == -1) break;
> > +
> > + switch (c)
> > + {
> > + case 'g':
> > + if (!(subarch = di_system_subarch_analyze_guess()))
> > + return EXIT_FAILURE;
> > +
> > + goto print_subarch;
> > + break;
>
> I think it would be neater to set a flag and do the actual processing
> outside the getopt loop.
>
> hw-detect will also need a build-dependency on a new enough
> libdebian-installer4-dev.
>
> Thanks,
>
> --
> Colin Watson [cjwatson@debian.org]
Reply to: