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

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

[Maciej Małecki]

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;
+		}
 	}
 
 	cxt->dev_fd = fd;
-- 
1.9.0

[PATCH 2/2] fdisk: do not allow writing to a read-only device

[Maciej Małecki]

Print a warning instead. This fixes the aforementioned "Bad file
descriptor" issue when writing to read-only completely.

Signed-off-by: Maciej Małecki <me@mmalecki.com>
---
 fdisks/fdisk-menu.c    | 4 ++++
 libfdisk/src/context.c | 3 +++
 libfdisk/src/fdiskP.h  | 1 +
 3 files changed, 8 insertions(+)

diff --git a/fdisks/fdisk-menu.c b/fdisks/fdisk-menu.c
index ced819a..f965e3f 100644
--- a/fdisks/fdisk-menu.c
+++ b/fdisks/fdisk-menu.c
@@ -448,6 +448,10 @@ static int generic_menu_cb(struct fdisk_context **cxt0,
 		rc = fdisk_list_disklabel(cxt);
 		break;
 	case 'w':
+		if (cxt->readonly) {
+			fdisk_warnx(cxt, _("Device open in read-only mode"));
+			break;
+		}
 		rc = fdisk_write_disklabel(cxt);
 		if (rc)
 			err(EXIT_FAILURE, _("failed to write disklabel"));
diff --git a/libfdisk/src/context.c b/libfdisk/src/context.c
index 40f9080..05833df 100644
--- a/libfdisk/src/context.c
+++ b/libfdisk/src/context.c
@@ -45,6 +45,7 @@ struct fdisk_context *fdisk_new_nested_context(struct fdisk_context *parent,
 	DBG(LABEL, dbgprint("new context %p allocated", cxt));
 	cxt->dev_fd = parent->dev_fd;
 	cxt->parent = parent;
+	cxt->readonly = parent->readonly;
 
 	cxt->io_size =          parent->io_size;
 	cxt->optimal_io_size =  parent->optimal_io_size;
@@ -173,6 +174,7 @@ static void reset_context(struct fdisk_context *cxt)
 
 	/* initialize */
 	cxt->dev_fd = -1;
+	cxt->readonly = -1;
 	cxt->dev_path = NULL;
 	cxt->firstsector = NULL;
 
@@ -271,6 +273,7 @@ retry:
 	}
 
 	cxt->dev_fd = fd;
+	cxt->readonly = readonly;
 	cxt->dev_path = strdup(fname);
 	if (!cxt->dev_path)
 		goto fail;
diff --git a/libfdisk/src/fdiskP.h b/libfdisk/src/fdiskP.h
index feaab3d..da56c13 100644
--- a/libfdisk/src/fdiskP.h
+++ b/libfdisk/src/fdiskP.h
@@ -264,6 +264,7 @@ struct fdisk_ask {
 
 struct fdisk_context {
 	int dev_fd;         /* device descriptor */
+	int readonly;       /* is the device read-only? */
 	char *dev_path;     /* device path */
 	unsigned char *firstsector; /* buffer with master boot record */
 
-- 
1.9.0

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

[Davidlohr Bueso]

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

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

[Maciej Małecki]

Davidlohr,

Thanks for the review.

I'm not sure why this is not a place for such message, as a user I
mostly open a device to write something to it. I want to know that
whatever I do, my writes will fail, otherwise I'm just wasting time
putting together my disk layout.
The reason for this patch was trying to open a write-blocked SD card.

Anyway, if that's still a NAK, should I only make this patch so that
`fdisk_context_assign_device` sets `dev->readonly = 1`, without any
error message?

On Fri, Feb 28, 2014 at 3:32 AM, Davidlohr Bueso <davidlohr@hp.com> wrote:
> 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
>
>

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

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/28/2014 6:00 AM, Maciej Małecki wrote:
>>> 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.

You really want to tell the user that they aren't going to be able to
write up front.

I have to say this function was already broken.  If the caller
requests write access and that fails, then the call should fail.  It
should *not* automatically fall back to read-only without giving any
indication to the caller.  If the caller wants to retry for read only,
then it can, and print a warning letting the user know it has switched
to ready only.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTELgMAAoJEI5FoCIzSKrwlM0H/iVJLdAPqE3OOgaIJRmk+NC5
uLlMwVMY4zHJakuq4GtxX5rekdy+kLTuVokZjxUTcI2WM9uBJwFXSwd4sPJ2i3C+
47w8tS4FMzP4LKOck60cc61FpAaKaqez9GGQ8zkMNZnt5S6ptCOOoMGtYuqXm1AW
LWYHSbSsmXsTiQIWLEg/GZhBMUaInjcMiVr4nzINXN8CnwJ695Xyold0rnkAHxK0
+FhKNtGuab88GjGosla9kK1Nd2/6rjLt6HqYDBHfZaWkcTzjv4y+OCEz6Am7SFZp
PkrqmWV0FXvUGj8C4RrNf3dSiElorgkPbTS1P1+JRNFLZIAO7YFbapke7MZ4eDg=
=7pzO
-----END PGP SIGNATURE-----

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

[Maciej Małecki]

Phillip,

That's pretty much what I thought about this function, but didn't want
to make too intrusive changes. I'll make this patch do that instead
(i.e. warn and retry opening in read-only mode in `fdisk`land, not
`libfdisk`land). Thanks for the review.

On Fri, Feb 28, 2014 at 5:23 PM, Phillip Susi <psusi@ubuntu.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 2/28/2014 6:00 AM, Maciej Małecki wrote:
>>>> 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.
>
> You really want to tell the user that they aren't going to be able to
> write up front.
>
> I have to say this function was already broken.  If the caller
> requests write access and that fails, then the call should fail.  It
> should *not* automatically fall back to read-only without giving any
> indication to the caller.  If the caller wants to retry for read only,
> then it can, and print a warning letting the user know it has switched
> to ready only.
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.17 (MingW32)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQEcBAEBAgAGBQJTELgMAAoJEI5FoCIzSKrwlM0H/iVJLdAPqE3OOgaIJRmk+NC5
> uLlMwVMY4zHJakuq4GtxX5rekdy+kLTuVokZjxUTcI2WM9uBJwFXSwd4sPJ2i3C+
> 47w8tS4FMzP4LKOck60cc61FpAaKaqez9GGQ8zkMNZnt5S6ptCOOoMGtYuqXm1AW
> LWYHSbSsmXsTiQIWLEg/GZhBMUaInjcMiVr4nzINXN8CnwJ695Xyold0rnkAHxK0
> +FhKNtGuab88GjGosla9kK1Nd2/6rjLt6HqYDBHfZaWkcTzjv4y+OCEz6Am7SFZp
> PkrqmWV0FXvUGj8C4RrNf3dSiElorgkPbTS1P1+JRNFLZIAO7YFbapke7MZ4eDg=
> =7pzO
> -----END PGP SIGNATURE-----

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

[Davidlohr Bueso]

On Fri, 2014-02-28 at 11:23 -0500, Phillip Susi wrote:
> On 2/28/2014 6:00 AM, Maciej Małecki wrote:
> >>> 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.
> 
> You really want to tell the user that they aren't going to be able to
> write up front.

Sure why not, just not in a library! You can do this in the fdisk
program. And if Karel doesn't agree and wants to apply this patch
anyway, then it should *at least* be in DBG context.

Thanks,
Davidlohr

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

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/28/2014 1:27 PM, Davidlohr Bueso wrote:
>> You really want to tell the user that they aren't going to be
>> able to write up front.
> 
> Sure why not, just not in a library! You can do this in the fdisk 
> program. And if Karel doesn't agree and wants to apply this patch 
> anyway, then it should *at least* be in DBG context.

Right, which is what I said just after that ;)


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTEOEAAAoJEI5FoCIzSKrwgosH/2VJe8a639As2jvFhBDnp8Fr
H0xuH1fPt/RTE3+d9GlCHlBsfWQlmBgNEzItJR2hCnHLikf8ghY8SXwCXqgmUYZa
RGdwyU10ulhBC552F5x2I+Qgu7xGRxsOhHOlxpgzrPtsZNkf5vqXwlNjxjGr2i2G
zLcdH4Oq2K/D0JoPEKS5Bn8xnbNL/vnd7B6Hn9tqZ8+RfPm6iajr5bIPGOpLOa2E
S/FBo+8Abjln2DIllSZGwQLsoidyWv3fT2iGxaMmtkAGNn9Dbw/r7Uvq4Yt1OICB
/T7F3y1fYb+E/1JPaNneM0wl2smyl3em40uKBuGIi4rhj0g2QVycmANOmol1P0U=
=rQOh
-----END PGP SIGNATURE-----

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

[Karel Zak]

On Fri, Feb 28, 2014 at 02:18:24PM -0500, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 2/28/2014 1:27 PM, Davidlohr Bueso wrote:
> >> You really want to tell the user that they aren't going to be
> >> able to write up front.
> > 
> > Sure why not, just not in a library! You can do this in the fdisk 
> > program. And if Karel doesn't agree and wants to apply this patch 
> > anyway, then it should *at least* be in DBG context.
> 
> Right, which is what I said just after that ;)

 I'll try to improve the function after libfdisk branch merge into
 master branch (this or next week). The current code is probably
 really not optimal.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

[PATCH v2 1/3] fdisk: do not allow writing to a read-only device

[Maciej Małecki]

Print a warning instead. This fixes the aforementioned "Bad file
descriptor" issue when writing to read-only completely.

Signed-off-by: Maciej Małecki <me@mmalecki.com>
---
 fdisks/fdisk-menu.c    | 4 ++++
 libfdisk/src/context.c | 3 +++
 libfdisk/src/fdiskP.h  | 1 +
 3 files changed, 8 insertions(+)

diff --git a/fdisks/fdisk-menu.c b/fdisks/fdisk-menu.c
index ced819a..f965e3f 100644
--- a/fdisks/fdisk-menu.c
+++ b/fdisks/fdisk-menu.c
@@ -448,6 +448,10 @@ static int generic_menu_cb(struct fdisk_context **cxt0,
 		rc = fdisk_list_disklabel(cxt);
 		break;
 	case 'w':
+		if (cxt->readonly) {
+			fdisk_warnx(cxt, _("Device open in read-only mode"));
+			break;
+		}
 		rc = fdisk_write_disklabel(cxt);
 		if (rc)
 			err(EXIT_FAILURE, _("failed to write disklabel"));
diff --git a/libfdisk/src/context.c b/libfdisk/src/context.c
index c405403..dc9f0f9 100644
--- a/libfdisk/src/context.c
+++ b/libfdisk/src/context.c
@@ -45,6 +45,7 @@ struct fdisk_context *fdisk_new_nested_context(struct fdisk_context *parent,
 	DBG(LABEL, dbgprint("new context %p allocated", cxt));
 	cxt->dev_fd = parent->dev_fd;
 	cxt->parent = parent;
+	cxt->readonly = parent->readonly;
 
 	cxt->io_size =          parent->io_size;
 	cxt->optimal_io_size =  parent->optimal_io_size;
@@ -173,6 +174,7 @@ static void reset_context(struct fdisk_context *cxt)
 
 	/* initialize */
 	cxt->dev_fd = -1;
+	cxt->readonly = -1;
 	cxt->dev_path = NULL;
 	cxt->firstsector = NULL;
 
@@ -262,6 +264,7 @@ int fdisk_context_assign_device(struct fdisk_context *cxt,
 	}
 
 	cxt->dev_fd = fd;
+	cxt->readonly = readonly;
 	cxt->dev_path = strdup(fname);
 	if (!cxt->dev_path)
 		goto fail;
diff --git a/libfdisk/src/fdiskP.h b/libfdisk/src/fdiskP.h
index feaab3d..da56c13 100644
--- a/libfdisk/src/fdiskP.h
+++ b/libfdisk/src/fdiskP.h
@@ -264,6 +264,7 @@ struct fdisk_ask {
 
 struct fdisk_context {
 	int dev_fd;         /* device descriptor */
+	int readonly;       /* is the device read-only? */
 	char *dev_path;     /* device path */
 	unsigned char *firstsector; /* buffer with master boot record */
 
-- 
1.9.0

[PATCH v2 2/3] libfdisk: remove `O_RDONLY` fallback mode when opening device

[Maciej Małecki]

---
 libfdisk/src/context.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/libfdisk/src/context.c b/libfdisk/src/context.c
index dc9f0f9..795718c 100644
--- a/libfdisk/src/context.c
+++ b/libfdisk/src/context.c
@@ -241,10 +241,6 @@ static int warn_wipe(struct fdisk_context *cxt)
  * @fname: path to the device to be handled
  * @readonly: how to open the device
  *
- * If the @readonly flag is set to false, fdisk will attempt to open
- * the device with read-write mode and will fallback to read-only if
- * unsuccessful.
- *
  * Returns: 0 on success, < 0 on error.
  */
 int fdisk_context_assign_device(struct fdisk_context *cxt,
@@ -257,10 +253,8 @@ int fdisk_context_assign_device(struct fdisk_context *cxt,
 
 	reset_context(cxt);
 
-	if (readonly == 1 || (fd = open(fname, O_RDWR|O_CLOEXEC)) < 0) {
-		if ((fd = open(fname, O_RDONLY|O_CLOEXEC)) < 0)
-			return -errno;
-		readonly = 1;
+	if ((fd = open(fname, (readonly == 1 ? O_RDONLY : O_RDWR) | O_CLOEXEC)) < 0) {
+		goto fail;
 	}
 
 	cxt->dev_fd = fd;
-- 
1.9.0

[PATCH v2 3/3] fdisk: retry opening device in read-only mode

[Maciej Małecki]

Print a warning if opening in write mode fails.
---
 fdisks/fdisk.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fdisks/fdisk.c b/fdisks/fdisk.c
index 7afd642..1c7b1f7 100644
--- a/fdisks/fdisk.c
+++ b/fdisks/fdisk.c
@@ -538,8 +538,16 @@ int main(int argc, char **argv)
 		fdisk_info(cxt, _("Changes will remain in memory only, until you decide to write them.\n"
 				  "Be careful before using the write command.\n"));
 
-		if (fdisk_context_assign_device(cxt, argv[optind], 0) != 0)
-			err(EXIT_FAILURE, _("cannot open %s"), argv[optind]);
+		if (fdisk_context_assign_device(cxt, argv[optind], 0) != 0) {
+			if (fdisk_context_assign_device(cxt, argv[optind], 1) != 0)
+				err(EXIT_FAILURE, _("cannot open %s"), argv[optind]);
+
+			fdisk_warnx(cxt, _(
+				  "Opening device in write mode failed, device "
+				  "is open in read-only mode.\nfdisk won't be "
+				  "able to write changes."));
+			assert(cxt->readonly);
+		}
 
 		fflush(stdout);
 
-- 
1.9.0

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

[Maciej Małecki]

I just sent out fixed patches, but for some reason they went to the
main list (I think I needed `git send-email --thread`). Sorry for
that.

Now `fdisk` binary falls back to retrying
`fdisk_context_assign_device` with `readonly == 1` and warns user
about it.

On Mon, Mar 3, 2014 at 10:04 AM, Karel Zak <kzak@redhat.com> wrote:
> On Fri, Feb 28, 2014 at 02:18:24PM -0500, Phillip Susi wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 2/28/2014 1:27 PM, Davidlohr Bueso wrote:
>> >> You really want to tell the user that they aren't going to be
>> >> able to write up front.
>> >
>> > Sure why not, just not in a library! You can do this in the fdisk
>> > program. And if Karel doesn't agree and wants to apply this patch
>> > anyway, then it should *at least* be in DBG context.
>>
>> Right, which is what I said just after that ;)
>
>  I'll try to improve the function after libfdisk branch merge into
>  master branch (this or next week). The current code is probably
>  really not optimal.
>
>     Karel
>
> --
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com

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

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/3/2014 4:19 PM, Maciej Małecki wrote:
> I just sent out fixed patches, but for some reason they went to
> the main list (I think I needed `git send-email --thread`). Sorry
> for that.

You want --in-reply-to and then you need to look up the Message-ID in
the headers of the mail you want to reply to.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTFPhuAAoJEI5FoCIzSKrwKqMIAI+ojKYXo/XGOnGf381PixCT
uMKDm1tcuWMrAkGQgJmKqqADIXqWyj15uiXu3P/d7eMEWEBgfEQjt0fwAzN6HWXg
CuO9vN2nJQhKh2iq7J+J5LPd7277dDN6QfZcu702OWA+FJlawNSrEejNUsONifNB
+ieQuh0e7b2LzPZmDhPJJVA+kD7vMYh0CGdVL4qgujP08EzN3DlS2lOg4s2TMBrQ
LeJPXAi65LahwBHCx08rJk5ub0rY+Zz3qlx7lus+AgW1xVKsqH7EJdiquR71OfhI
pAVAR71r/Xg7xOi1mD0KvsoM/tBk5Viu93ubnKCYpZJtcBjHCpgAOTHPLgjz4O4=
=HE/O
-----END PGP SIGNATURE-----

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

[Karel Zak]

On Fri, Feb 28, 2014 at 11:23:40AM -0500, Phillip Susi wrote:
> You really want to tell the user that they aren't going to be able to
> write up front.
> 
> I have to say this function was already broken.  If the caller
> requests write access and that fails, then the call should fail.  It
> should *not* automatically fall back to read-only without giving any
> indication to the caller.  If the caller wants to retry for read only,
> then it can, and print a warning letting the user know it has switched
> to ready only.

 I agree, implemented. The function does not fallback to read-only mode
 now -- all has to be controlled by caller.

 Thanks Maciej for your report!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com