[PATCH] fix grub-setup on kfreebsd by adding 0x10 to the sysctl ("kern.geom.debugflags") flags

[PATCH] fix grub-setup on kfreebsd by adding 0x10 to the sysctl ("kern.geom.debugflags") flags

[Felix Zielcke]

Here's a patch to fix grub-setup on Debian GNU/kFreeBSD.
Without it, it fails always with `grub-setup: error: cannot open
`/dev/da0' in open_device()'
With it, it works fine.

-- 
Felix Zielcke

Re: [PATCH] fix grub-setup on kfreebsd by adding 0x10 to the sysctl ("kern.geom.debugflags") flags

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

Am Montag, den 13.04.2009, 15:32 +0200 schrieb Felix Zielcke:
> Here's a patch to fix grub-setup on Debian GNU/kFreeBSD.
> Without it, it fails always with `grub-setup: error: cannot open
> `/dev/da0' in open_device()'
> With it, it works fine.

Args there should be a patch attached. Fixed.
-- 
Felix Zielcke

[-- Attachment #2: kfreebsd.patch --]
[-- Type: text/x-patch, Size: 1672 bytes --]

2009-04-13  Felix Zielcke  <fzielcke@z-51.de>

	util/hostdisk.c [__FreeBSD_kernel]: Include <sys/param.h> and
	<sys/sysctl.h>
	(open_device) [__FreeBSD_kernel_]: Use sysctlgetbyname() to add 0x10 to
	`kern.geom.debugflags', before opening the device and reset them afterwards.

Index: util/hostdisk.c
===================================================================
--- util/hostdisk.c	(revision 2094)
+++ util/hostdisk.c	(working copy)
@@ -90,6 +90,11 @@ struct hd_geometry
 # include <sys/disk.h> /* DIOCGMEDIASIZE */
 #endif
 
+#ifdef __FreeBSD_kernel__
+#include <sys/param.h>
+#include <sys/sysctl.h>
+#endif
+
 struct
 {
   char *drive;
@@ -340,7 +345,24 @@ open_device (const grub_disk_t disk, gru
       sector -= disk->partition->start;
   }
 #else /* ! __linux__ */
+#if defined __FreeBSD_kernel__
+  int sysctl_flags, sysctl_oldflags;
+  size_t sysctl_size = sizeof (sysctl_flags);
+
+  if (sysctlbyname ("kern.geom.debugflags", &sysctl_oldflags, &sysctl_size, NULL, 0))
+    grub_util_error ("cannot get current flags of sysctl kern.geom.debugflags");
+  sysctl_flags = sysctl_oldflags |= 0x10;
+  if (sysctlbyname ("kern.geom.debugflags", NULL , 0, &sysctl_flags, sysctl_size))
+    grub_util_error ("cannot set flags of sysctl kern.geom.debugflags");
+#endif
+
   fd = open (map[disk->id].device, flags);
+
+#if defined __FreeBSD_kernel__
+  if (sysctlbyname ("kern.geom.debugflags", NULL , 0, &sysctl_oldflags, sysctl_size))
+    grub_util_error ("cannot set flags back to the old value for sysctl kern.geom.debugflags");
+#endif
+
   if (fd < 0)
     {
       grub_error (GRUB_ERR_BAD_DEVICE, "cannot open `%s' in open_device()", map[disk->id].device);

Re: [PATCH] fix grub-setup on kfreebsd by adding 0x10 to the sysctl ("kern.geom.debugflags") flags

[Robert Millan]

On Mon, Apr 13, 2009 at 04:26:19PM +0200, Felix Zielcke wrote:
> +#ifdef __FreeBSD_kernel__

Unfortunately FreeBSD doesn't define __FreeBSD_kernel__, so we need
to check for both macros.

> +#if defined __FreeBSD_kernel__
> +  int sysctl_flags, sysctl_oldflags;
> +  size_t sysctl_size = sizeof (sysctl_flags);

You can add a `const' on this one.

> +  sysctl_flags = sysctl_oldflags |= 0x10;

Shouldn't this be "sysctl_oldflags | 0x10" ?

Btw you can avoid the two subsequent calls if sysctl_flags == sysctl_oldflags.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] fix grub-setup on kfreebsd by adding 0x10 to the sysctl ("kern.geom.debugflags") flags

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

Am Montag, den 13.04.2009, 16:49 +0200 schrieb Robert Millan:

Thanks for reviewing.

> On Mon, Apr 13, 2009 at 04:26:19PM +0200, Felix Zielcke wrote:
> > +#ifdef __FreeBSD_kernel__
> 
> Unfortunately FreeBSD doesn't define __FreeBSD_kernel__, so we need
> to check for both macros.

Oh right.

> > +#if defined __FreeBSD_kernel__
> > +  int sysctl_flags, sysctl_oldflags;
> > +  size_t sysctl_size = sizeof (sysctl_flags);
> 
> You can add a `const' on this one.
> 
> > +  sysctl_flags = sysctl_oldflags |= 0x10;
> 
> Shouldn't this be "sysctl_oldflags | 0x10" ?

Yes.
> Btw you can avoid the two subsequent calls if sysctl_flags == sysctl_oldflags.

Right and actually it can be even avoided if 0x10 is already set.

Is it correct that I used now `[__FreeBSD__]: Likewise.' in the
Changelog?
The GCS unfortunately doestn't tell anything about the case that a
change applies to 2 or more marcros and I couldn't find an example in
the existing Changelog for this.

-- 
Felix Zielcke

[-- Attachment #2: kfreebsd.patch.2 --]
[-- Type: text/plain, Size: 1865 bytes --]

2009-04-13  Felix Zielcke  <fzielcke@z-51.de>

	util/hostdisk.c [__FreeBSD_kernel]: Include <sys/param.h> and
	<sys/sysctl.h>.
	[__FreeBSD__]: Likewise.
	(open_device) [__FreeBSD_kernel_]: Use sysctlgetbyname() to add 0x10 to
	`kern.geom.debugflags' if it's not already set, before opening the
	device and reset them afterwards.
	[__FreeBSD__]: Likewise.

Index: util/hostdisk.c
===================================================================
--- util/hostdisk.c	(revision 2094)
+++ util/hostdisk.c	(working copy)
@@ -88,6 +88,8 @@ struct hd_geometry
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 # include <sys/disk.h> /* DIOCGMEDIASIZE */
+# include <sys/param.h>
+# include <sys/sysctl.h>
 #endif
 
 struct
@@ -340,7 +342,24 @@ open_device (const grub_disk_t disk, gru
       sector -= disk->partition->start;
   }
 #else /* ! __linux__ */
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
+  int sysctl_flags, sysctl_oldflags;
+  const size_t sysctl_size = sizeof (sysctl_flags);
+
+  if (sysctlbyname ("kern.geom.debugflags", &sysctl_oldflags, &sysctl_size, NULL, 0))
+    grub_util_error ("cannot get current flags of sysctl kern.geom.debugflags");
+  sysctl_flags = sysctl_oldflags | 0x10;
+  if (! sysctl_oldflags & 0x10 && sysctlbyname ("kern.geom.debugflags", NULL , 0, &sysctl_flags, sysctl_size))
+    grub_util_error ("cannot set flags of sysctl kern.geom.debugflags");
+#endif
+
   fd = open (map[disk->id].device, flags);
+
+#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
+  if (! sysctl_oldflags & 0x10 && sysctlbyname ("kern.geom.debugflags", NULL , 0, &sysctl_oldflags, sysctl_size))
+    grub_util_error ("cannot set flags back to the old value for sysctl kern.geom.debugflags");
+#endif
+
   if (fd < 0)
     {
       grub_error (GRUB_ERR_BAD_DEVICE, "cannot open `%s' in open_device()", map[disk->id].device);

Re: [PATCH] fix grub-setup on kfreebsd by adding 0x10 to the sysctl ("kern.geom.debugflags") flags

[Robert Millan]

On Mon, Apr 13, 2009 at 05:11:04PM +0200, Felix Zielcke wrote:
> > Btw you can avoid the two subsequent calls if sysctl_flags == sysctl_oldflags.
> 
> Right and actually it can be even avoided if 0x10 is already set.

It's the same thing (if and only if 0x10 was set, old and new flags will
be the same).  I'm not sure which check is cheaper, but it doesn't matter
much anyway.

> Is it correct that I used now `[__FreeBSD__]: Likewise.' in the
> Changelog?
> The GCS unfortunately doestn't tell anything about the case that a
> change applies to 2 or more marcros and I couldn't find an example in
> the existing Changelog for this.

How about [__FreeBSD__ || __FreeBSD_kernel__] ?

> +  if (sysctlbyname ("kern.geom.debugflags", &sysctl_oldflags, &sysctl_size, NULL, 0))
> +    grub_util_error ("cannot get current flags of sysctl kern.geom.debugflags");

I'd just return grub_error instead.  Otherwise we abort the program even if
failure to read a drive is not critical (e.g. lvm.mod scannning all drives,
grub-emu, etc).

> +  if (! sysctl_oldflags & 0x10 && sysctlbyname ("kern.geom.debugflags", NULL , 0, &sysctl_flags, sysctl_size))
> +    grub_util_error ("cannot set flags of sysctl kern.geom.debugflags");

Just a matter of taste, I'd suggest nested ifs to make it more readable.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] fix grub-setup on kfreebsd by adding 0x10 to the sysctl ("kern.geom.debugflags") flags

[Felix Zielcke]

Am Montag, den 13.04.2009, 21:11 +0200 schrieb Robert Millan:

> How about [__FreeBSD__ || __FreeBSD_kernel__] ?

Ok.

> > +  if (sysctlbyname ("kern.geom.debugflags", &sysctl_oldflags, &sysctl_size, NULL, 0))
> > +    grub_util_error ("cannot get current flags of sysctl kern.geom.debugflags");
> 
> I'd just return grub_error instead.  Otherwise we abort the program even if
> failure to read a drive is not critical (e.g. lvm.mod scannning all drives,
> grub-emu, etc).

Ok.

> > +  if (! sysctl_oldflags & 0x10 && sysctlbyname ("kern.geom.debugflags", NULL , 0, &sysctl_flags, sysctl_size))
> > +    grub_util_error ("cannot set flags of sysctl kern.geom.debugflags");
> 
> Just a matter of taste, I'd suggest nested ifs to make it more readable.

Changed too.
I commited this now.
-- 
Felix Zielcke