Re: [PATCH 1/2] fdisk: warn if opening a device in write mode failed

[Davidlohr Bueso <davidlohr@hp.com>]

On Thu, 2014-02-27 at 04:26 +0100, Maciej Małecki wrote:
> Otherwise, `fdisk` fails silently with "Bad file descriptor" when
> writing the partition table.
> 
> Signed-off-by: Maciej Małecki <me@mmalecki.com>
> ---
>  libfdisk/src/context.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/libfdisk/src/context.c b/libfdisk/src/context.c
> index c405403..40f9080 100644
> --- a/libfdisk/src/context.c
> +++ b/libfdisk/src/context.c
> @@ -248,17 +248,26 @@ static int warn_wipe(struct fdisk_context *cxt)
>  int fdisk_context_assign_device(struct fdisk_context *cxt,
>  				const char *fname, int readonly)
>  {
> -	int fd;
> +	int fd, mode;
>  
>  	DBG(CONTEXT, dbgprint("assigning device %s", fname));
>  	assert(cxt);
>  
>  	reset_context(cxt);
>  
> -	if (readonly == 1 || (fd = open(fname, O_RDWR|O_CLOEXEC)) < 0) {
> -		if ((fd = open(fname, O_RDONLY|O_CLOEXEC)) < 0)
> +retry:
> +	mode = (readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC;
> +	fd = open(fname, mode);
> +	if (fd < 0) {
> +		if (readonly)
>  			return -errno;
> -		readonly = 1;
> +		else {
> +			fdisk_warn(cxt,
> +			    _("%s: opening device in read write mode failed"),
> +			    fname);
> +			readonly = 1;
> +			goto retry;
> +		}
>  	}

This is 1) obscenely ugly and 2) not the place for such message. There's
no reason to pollute output like this. Why do I care how the device is
opened when I'm not going to write anything to it?

The correct way is to do what you do in patch 2, inform the user that
the device is only for opened for reading and just skip the write
operation.

So, NAK for this patch.

Thanks,
Davidlohr