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@xxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
