Re: [PATCH] Remove DASD restriction on _disk_sync_part_table (#651478)

[Hans de Goede]

Hi,

I think the comment about devfs can and should be removed too, esp.
as we no longer test for devfs (and fallback to the reread ioctl in
case of devfs).

Regards,

Hans



On 11/22/2010 10:04 AM, Jim Meyering wrote:
> Brian C. Lane wrote:
> > ---
> >   libparted/arch/linux.c |   23 ++---------------------
> >   1 files changed, 2 insertions(+), 21 deletions(-)
> >
> > diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> > index 38f4e31..435cb46 100644
> > --- a/libparted/arch/linux.c
> > +++ b/libparted/arch/linux.c
> ...
> > @@ -2807,14 +2794,8 @@ linux_disk_commit (PedDisk* disk)
> >                    * created.  Therefore, if using DevFS, we must get the kernel
> >                    * to re-read and grok the partition table.
> >                    */
> > -                /* Work around kernel dasd problem so we really do BLKRRPART */
> > -               int ok = 1;
> > -               if (disk->dev->type != PED_DEVICE_DASD&&  _have_blkpg ()) {
> > -                       if (!_disk_sync_part_table (disk))
> > -                         ok = 0;
> > -               }
> > -
> > -                return ok;
> > +                if (!_disk_sync_part_table (disk))
> > +                    return 0;
> >           }
> >
> >           return 1;
>
> Thanks, Brian.
> I've adjusted your patch to retain _have_blkpg,
> so that if someone tries to build on pre-2.4.x, they'll
> get a failed assertion rather than subtle misbehavior,
> and added a note to remove the assertion in a year or two.
>
> I'll push something like the following after I write a test
> to exercise this fix.
>
>  From 1eb86eb4ea1f9961530cc4f209c3f468a6b6c73c Mon Sep 17 00:00:00 2001
> From: Brian C. Lane<address@hidden>
> Date: Mon, 22 Nov 2010 10:01:56 +0100
> Subject: [PATCH] linux: remove DASD restriction on _disk_sync_part_table
>
> * libparted/arch/linux.c: Include<assert.h>.
> (linux_disk_commit): Assert that _have_blkpg returns true
> and don't skip _disk_sync_part_table for DASD devices.
> Otherwise, when creating a partition on a DASD device,
> new /dev/*[12...] device files would not be created.
> For details, see http://bugzilla.redhat.com/651478.
> ---
>   libparted/arch/linux.c |   18 ++++++++++--------
>   1 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> index a5ae10c..0807c8b 100644
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -40,6 +40,7 @@
>   #include<sys/types.h>
>   #include<sys/utsname.h>         /* for uname() */
>   #include<scsi/scsi.h>
> +#include<assert.h>
>   #ifdef ENABLE_DEVICE_MAPPER
>   #include<libdevmapper.h>
>   #endif
> @@ -2824,20 +2825,21 @@ linux_disk_commit (PedDisk* disk)
>                   return _dm_reread_part_table (disk);
>   #endif
>           if (disk->dev->type != PED_DEVICE_FILE) {
> +
> +               /* We now require BLKPG support.  If this assertion fails,
> +                  please write to the mailing list describing your system.
> +                  Assuming it's never triggered, ...
> +                  FIXME: remove this assertion in 2012.  */
> +               assert (_have_blkpg ());
> +
>                   /* The ioctl() command BLKPG_ADD_PARTITION does not notify
>                    * the devfs system; consequently, /proc/partitions will not
>                    * be up to date, and the proper links in /dev are not
>                    * created.  Therefore, if using DevFS, we must get the kernel
>                    * to re-read and grok the partition table.
>                    */
> -                /* Work around kernel dasd problem so we really do BLKRRPART */
> -               int ok = 1;
> -               if (disk->dev->type != PED_DEVICE_DASD&&  _have_blkpg ()) {
> -                       if (!_disk_sync_part_table (disk))
> -                         ok = 0;
> -               }
> -
> -                return ok;
> +               if (!_disk_sync_part_table (disk))
> +                       return 0;
>           }
>
>           return 1;
> --
> 1.7.3.2.765.g642a8