* [RFC] common: filetype: is_fat_or_mbr() considered harmful @ 2015-10-07 18:03 Peter Mamonov 2015-10-09 8:06 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Peter Mamonov @ 2015-10-07 18:03 UTC (permalink / raw) To: barebox; +Cc: Peter Mamonov Deleted pieces of code detect MBR-containig device as a FAT-type device, if it's first partition contains a FAT filesystem. So, one can mount the first partition of a hard drive containing FAT FS using the following command: barebox: mount /dev/ata0.0 /mnt/0 as well as this one: barebox: mount /dev/ata0 /mnt/1 Both commands mount the same FS. This behaviour causes automount (mount -a) to mount FAT FS on a first partition twice: barebox: mount none on / type ramfs none on /dev type devfs /dev/ata0 on /mnt/ata0 type fat /dev/ata0.0 on /mnt/ata0.0 type fat /dev/ata0.1 on /mnt/ata0.1 type ext4 --- common/filetype.c | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/common/filetype.c b/common/filetype.c index cc3099b..49a0c8d 100644 --- a/common/filetype.c +++ b/common/filetype.c @@ -345,21 +345,6 @@ enum filetype file_name_detect_type(const char *filename) type = file_detect_type(buf, ret); - if (type == filetype_mbr) { - /* - * Get the first partition start sector - * and check for FAT in it - */ - is_fat_or_mbr(buf, &bootsec); - ret = lseek(fd, (bootsec) * 512, SEEK_SET); - if (ret < 0) - goto err_out; - ret = read(fd, buf, 512); - if (ret < 0) - goto err_out; - type = is_fat_or_mbr((u8 *)buf, NULL); - } - err_out: close(fd); free(buf); @@ -384,21 +369,6 @@ enum filetype cdev_detect_type(const char *name) type = file_detect_type(buf, ret); - if (type == filetype_mbr) { - unsigned long bootsec; - /* - * Get the first partition start sector - * and check for FAT in it - */ - is_fat_or_mbr(buf, &bootsec); - - ret = cdev_read(cdev, buf, 512, bootsec * 512, 0); - if (ret < 0) - goto err_out; - - type = is_fat_or_mbr((u8 *)buf, NULL); - } - err_out: free(buf); return type; -- 2.1.4 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-07 18:03 [RFC] common: filetype: is_fat_or_mbr() considered harmful Peter Mamonov @ 2015-10-09 8:06 ` Sascha Hauer 2015-10-09 12:40 ` Peter Mamonov 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2015-10-09 8:06 UTC (permalink / raw) To: Peter Mamonov; +Cc: barebox Hi Peter, On Wed, Oct 07, 2015 at 09:03:56PM +0300, Peter Mamonov wrote: > Deleted pieces of code detect MBR-containig device as a FAT-type device, > if it's first partition contains a FAT filesystem. So, one can mount the first > partition of a hard drive containing FAT FS using the following command: > barebox: mount /dev/ata0.0 /mnt/0 > as well as this one: > barebox: mount /dev/ata0 /mnt/1 > Both commands mount the same FS. > > This behaviour causes automount (mount -a) to mount FAT FS > on a first partition twice: > barebox: mount > none on / type ramfs > none on /dev type devfs > /dev/ata0 on /mnt/ata0 type fat > /dev/ata0.0 on /mnt/ata0.0 type fat > /dev/ata0.1 on /mnt/ata0.1 type ext4 This is_fat_or_mbr mechanism never worked very well and had funny side effects. Would be nice to get rid of it. Simply removing this option is not a solution though, we have to find a proper way to keep the current feature and make it more sane. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-09 8:06 ` Sascha Hauer @ 2015-10-09 12:40 ` Peter Mamonov 2015-10-09 16:11 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Peter Mamonov @ 2015-10-09 12:40 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On Fri, 9 Oct 2015 10:06:24 +0200 Sascha Hauer <s.hauer@pengutronix.de> wrote: > Hi Peter, > > On Wed, Oct 07, 2015 at 09:03:56PM +0300, Peter Mamonov wrote: > > Deleted pieces of code detect MBR-containig device as a FAT-type > > device, if it's first partition contains a FAT filesystem. So, one > > can mount the first partition of a hard drive containing FAT FS > > using the following command: barebox: mount /dev/ata0.0 /mnt/0 > > as well as this one: > > barebox: mount /dev/ata0 /mnt/1 > > Both commands mount the same FS. > > > > This behaviour causes automount (mount -a) to mount FAT FS > > on a first partition twice: > > barebox: mount > > none on / type ramfs > > none on /dev type devfs > > /dev/ata0 on /mnt/ata0 type fat > > /dev/ata0.0 on /mnt/ata0.0 type fat > > /dev/ata0.1 on /mnt/ata0.1 type ext4 > > This is_fat_or_mbr mechanism never worked very well and had funny side > effects. Would be nice to get rid of it. > Simply removing this option is not a solution though, we have to find > a proper way to keep the current feature and make it more sane. Ok, the patch comment is misleading a bit. I do not propose to get rid of the is_fat_or_mbr() completely. However, I do not see the point to check for a FAT FS, after the device was correctly detected as an MBR-type device: enum filetype file_name_detect_type(const char *filename) ... type = file_detect_type(buf, ret); if (type == filetype_mbr) { /* * Get the first partition start sector * and check for FAT in it */ is_fat_or_mbr(buf, &bootsec); ret = lseek(fd, (bootsec) * 512, SEEK_SET); if (ret < 0) goto err_out; ret = read(fd, buf, 512); if (ret < 0) goto err_out; type = is_fat_or_mbr((u8 *)buf, NULL); } The deleted code snippet was introduced by this patch: commit 010ee209b75c5732ae4144e3ee9ce14158193c1f Author: Franck Jullien <franck.jullien@gmail.com> Date: Wed Sep 19 13:09:01 2012 +0200 filetype: Improve FAT detection We may have some disk with MBR as a first sector. In this case, the current FAT check returns an error. However, the FAT sector exist and the MBR can tell us where it is. This patch add to file_name_detect_type function the ability to find the FAT boot sector on the first sector of the first partition in case it is not on sector 0. It also introduce is_fat_or_mbr to check if a buffer is a FAT boot or MBR sector Signed-off-by: Franck Jullien <franck.jullien@gmail.com> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> According to the patch message it was introduced to workaround FAT detection. However, after deletion of the code I'm still able to detect and mount FAT-containig partiotions. Peter > > Sascha > _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-09 12:40 ` Peter Mamonov @ 2015-10-09 16:11 ` Sascha Hauer 2015-10-12 10:27 ` Peter Mamonov 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2015-10-09 16:11 UTC (permalink / raw) To: Peter Mamonov; +Cc: barebox On Fri, Oct 09, 2015 at 03:40:37PM +0300, Peter Mamonov wrote: > On Fri, 9 Oct 2015 10:06:24 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > Hi Peter, > > > > On Wed, Oct 07, 2015 at 09:03:56PM +0300, Peter Mamonov wrote: > > > Deleted pieces of code detect MBR-containig device as a FAT-type > > > device, if it's first partition contains a FAT filesystem. So, one > > > can mount the first partition of a hard drive containing FAT FS > > > using the following command: barebox: mount /dev/ata0.0 /mnt/0 > > > as well as this one: > > > barebox: mount /dev/ata0 /mnt/1 > > > Both commands mount the same FS. > > > > > > This behaviour causes automount (mount -a) to mount FAT FS > > > on a first partition twice: > > > barebox: mount > > > none on / type ramfs > > > none on /dev type devfs > > > /dev/ata0 on /mnt/ata0 type fat > > > /dev/ata0.0 on /mnt/ata0.0 type fat > > > /dev/ata0.1 on /mnt/ata0.1 type ext4 > > > > This is_fat_or_mbr mechanism never worked very well and had funny side > > effects. Would be nice to get rid of it. > > Simply removing this option is not a solution though, we have to find > > a proper way to keep the current feature and make it more sane. > > Ok, the patch comment is misleading a bit. I do not propose to get rid > of the is_fat_or_mbr() completely. However, I do not see the point > to check for a FAT FS, after the device was correctly detected as an > MBR-type device: > > enum filetype file_name_detect_type(const char *filename) > ... > type = file_detect_type(buf, ret); > > if (type == filetype_mbr) { > /* > * Get the first partition start sector > * and check for FAT in it > */ > is_fat_or_mbr(buf, &bootsec); > ret = lseek(fd, (bootsec) * 512, SEEK_SET); > if (ret < 0) > goto err_out; > ret = read(fd, buf, 512); > if (ret < 0) > goto err_out; > type = is_fat_or_mbr((u8 *)buf, NULL); > } > > > The deleted code snippet was introduced by this patch: > > commit 010ee209b75c5732ae4144e3ee9ce14158193c1f > Author: Franck Jullien <franck.jullien@gmail.com> > Date: Wed Sep 19 13:09:01 2012 +0200 > > filetype: Improve FAT detection > > We may have some disk with MBR as a first sector. In this case, the > current FAT check returns an error. However, the FAT sector exist > and the MBR can tell us where it is. > > This patch add to file_name_detect_type function the ability to find > the FAT boot sector on the first sector of the first partition in > case it is not on sector 0. > > It also introduce is_fat_or_mbr to check if a buffer is a FAT boot > or MBR sector > > Signed-off-by: Franck Jullien <franck.jullien@gmail.com> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > According to the patch message it was introduced to workaround FAT > detection. However, after deletion of the code I'm still able to detect > and mount FAT-containig partiotions. But can you mount /dev/disk0 if this disk contains a partition table and the FAT is on /dev/disk0.0? This is what the patch is about. The problem the patches solved is that when you plug in a USB drive then you don't know whether a FAT is directly on the device or if the device is partitioned. You want to be able to mount both ways with the same command, so no matter if the FAT is on /dev/disk0 or /dev/disk0.0 you can mount both using /dev/disk0. The way this problem is solved currently is not very good, we should find a better way. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-09 16:11 ` Sascha Hauer @ 2015-10-12 10:27 ` Peter Mamonov 2015-10-12 13:51 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Peter Mamonov @ 2015-10-12 10:27 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On Fri, 9 Oct 2015 18:11:44 +0200 Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Fri, Oct 09, 2015 at 03:40:37PM +0300, Peter Mamonov wrote: > > On Fri, 9 Oct 2015 10:06:24 +0200 > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > Hi Peter, > > > > > > On Wed, Oct 07, 2015 at 09:03:56PM +0300, Peter Mamonov wrote: > > > > Deleted pieces of code detect MBR-containig device as a FAT-type > > > > device, if it's first partition contains a FAT filesystem. So, > > > > one can mount the first partition of a hard drive containing > > > > FAT FS using the following command: barebox: > > > > mount /dev/ata0.0 /mnt/0 as well as this one: > > > > barebox: mount /dev/ata0 /mnt/1 > > > > Both commands mount the same FS. > > > > > > > > This behaviour causes automount (mount -a) to mount FAT FS > > > > on a first partition twice: > > > > barebox: mount > > > > none on / type ramfs > > > > none on /dev type devfs > > > > /dev/ata0 on /mnt/ata0 type fat > > > > /dev/ata0.0 on /mnt/ata0.0 type fat > > > > /dev/ata0.1 on /mnt/ata0.1 type ext4 > > > > > > This is_fat_or_mbr mechanism never worked very well and had funny > > > side effects. Would be nice to get rid of it. > > > Simply removing this option is not a solution though, we have to > > > find a proper way to keep the current feature and make it more > > > sane. > > > > Ok, the patch comment is misleading a bit. I do not propose to get > > rid of the is_fat_or_mbr() completely. However, I do not see the > > point to check for a FAT FS, after the device was correctly > > detected as an MBR-type device: > > > > enum filetype file_name_detect_type(const char *filename) > > ... > > type = file_detect_type(buf, ret); > > > > if (type == filetype_mbr) { > > /* > > * Get the first partition start sector > > * and check for FAT in it > > */ > > is_fat_or_mbr(buf, &bootsec); > > ret = lseek(fd, (bootsec) * 512, SEEK_SET); > > if (ret < 0) > > goto err_out; > > ret = read(fd, buf, 512); > > if (ret < 0) > > goto err_out; > > type = is_fat_or_mbr((u8 *)buf, NULL); > > } > > > > > > The deleted code snippet was introduced by this patch: > > > > commit 010ee209b75c5732ae4144e3ee9ce14158193c1f > > Author: Franck Jullien <franck.jullien@gmail.com> > > Date: Wed Sep 19 13:09:01 2012 +0200 > > > > filetype: Improve FAT detection > > > > We may have some disk with MBR as a first sector. In this case, > > the current FAT check returns an error. However, the FAT sector > > exist and the MBR can tell us where it is. > > > > This patch add to file_name_detect_type function the ability to > > find the FAT boot sector on the first sector of the first partition > > in case it is not on sector 0. > > > > It also introduce is_fat_or_mbr to check if a buffer is a FAT > > boot or MBR sector > > > > Signed-off-by: Franck Jullien <franck.jullien@gmail.com> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > According to the patch message it was introduced to workaround FAT > > detection. However, after deletion of the code I'm still able to > > detect and mount FAT-containig partiotions. > > But can you mount /dev/disk0 if this disk contains a partition table > and the FAT is on /dev/disk0.0? No. This is actually the purpose of my patch, since I don't want "mount -a" to mount the same partition (FAT on /dev/disk0.0) twice. > This is what the patch is about. The > problem the patches solved is that when you plug in a USB drive then > you don't know whether a FAT is directly on the device or if the > device is partitioned. You want to be able to mount both ways with > the same command, so no matter if the FAT is on /dev/disk0 > or /dev/disk0.0 you can mount both using /dev/disk0. Ok. So what is the preferred way to prevent "mount -a" from mounting /dev/disk0 and /dev/disk0.0 at the same time? > The way this problem is solved currently is not very good, we should > find a better way. > > Sascha > _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-12 10:27 ` Peter Mamonov @ 2015-10-12 13:51 ` Sascha Hauer 2015-10-12 14:21 ` Antony Pavlov 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2015-10-12 13:51 UTC (permalink / raw) To: Peter Mamonov; +Cc: barebox On Mon, Oct 12, 2015 at 01:27:35PM +0300, Peter Mamonov wrote: > On Fri, 9 Oct 2015 18:11:44 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > On Fri, Oct 09, 2015 at 03:40:37PM +0300, Peter Mamonov wrote: > > > On Fri, 9 Oct 2015 10:06:24 +0200 > > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > > > Hi Peter, > > > > > > > > On Wed, Oct 07, 2015 at 09:03:56PM +0300, Peter Mamonov wrote: > > > > > Deleted pieces of code detect MBR-containig device as a FAT-type > > > > > device, if it's first partition contains a FAT filesystem. So, > > > > > one can mount the first partition of a hard drive containing > > > > > FAT FS using the following command: barebox: > > > > > mount /dev/ata0.0 /mnt/0 as well as this one: > > > > > barebox: mount /dev/ata0 /mnt/1 > > > > > Both commands mount the same FS. > > > > > > > > > > This behaviour causes automount (mount -a) to mount FAT FS > > > > > on a first partition twice: > > > > > barebox: mount > > > > > none on / type ramfs > > > > > none on /dev type devfs > > > > > /dev/ata0 on /mnt/ata0 type fat > > > > > /dev/ata0.0 on /mnt/ata0.0 type fat > > > > > /dev/ata0.1 on /mnt/ata0.1 type ext4 > > > > > > > > This is_fat_or_mbr mechanism never worked very well and had funny > > > > side effects. Would be nice to get rid of it. > > > > Simply removing this option is not a solution though, we have to > > > > find a proper way to keep the current feature and make it more > > > > sane. > > > > > > Ok, the patch comment is misleading a bit. I do not propose to get > > > rid of the is_fat_or_mbr() completely. However, I do not see the > > > point to check for a FAT FS, after the device was correctly > > > detected as an MBR-type device: > > > > > > enum filetype file_name_detect_type(const char *filename) > > > ... > > > type = file_detect_type(buf, ret); > > > > > > if (type == filetype_mbr) { > > > /* > > > * Get the first partition start sector > > > * and check for FAT in it > > > */ > > > is_fat_or_mbr(buf, &bootsec); > > > ret = lseek(fd, (bootsec) * 512, SEEK_SET); > > > if (ret < 0) > > > goto err_out; > > > ret = read(fd, buf, 512); > > > if (ret < 0) > > > goto err_out; > > > type = is_fat_or_mbr((u8 *)buf, NULL); > > > } > > > > > > > > > The deleted code snippet was introduced by this patch: > > > > > > commit 010ee209b75c5732ae4144e3ee9ce14158193c1f > > > Author: Franck Jullien <franck.jullien@gmail.com> > > > Date: Wed Sep 19 13:09:01 2012 +0200 > > > > > > filetype: Improve FAT detection > > > > > > We may have some disk with MBR as a first sector. In this case, > > > the current FAT check returns an error. However, the FAT sector > > > exist and the MBR can tell us where it is. > > > > > > This patch add to file_name_detect_type function the ability to > > > find the FAT boot sector on the first sector of the first partition > > > in case it is not on sector 0. > > > > > > It also introduce is_fat_or_mbr to check if a buffer is a FAT > > > boot or MBR sector > > > > > > Signed-off-by: Franck Jullien <franck.jullien@gmail.com> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > > > According to the patch message it was introduced to workaround FAT > > > detection. However, after deletion of the code I'm still able to > > > detect and mount FAT-containig partiotions. > > > > But can you mount /dev/disk0 if this disk contains a partition table > > and the FAT is on /dev/disk0.0? > > No. This is actually the purpose of my patch, since I don't want > "mount -a" to mount the same partition (FAT on /dev/disk0.0) twice. I know, and this is valid. It just conflicts with what Franck wants. He just wants to mount a USB device without having to know if the FAT is on the raw device or on the first partition. > > > This is what the patch is about. The > > problem the patches solved is that when you plug in a USB drive then > > you don't know whether a FAT is directly on the device or if the > > device is partitioned. You want to be able to mount both ways with > > the same command, so no matter if the FAT is on /dev/disk0 > > or /dev/disk0.0 you can mount both using /dev/disk0. > > Ok. So what is the preferred way to prevent "mount -a" from mounting > /dev/disk0 and /dev/disk0.0 at the same time? Sorry, I do not have a solution currently. I'll have a look into it. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-12 13:51 ` Sascha Hauer @ 2015-10-12 14:21 ` Antony Pavlov 2015-10-12 14:37 ` Franck Jullien 0 siblings, 1 reply; 9+ messages in thread From: Antony Pavlov @ 2015-10-12 14:21 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox, Peter Mamonov On Mon, 12 Oct 2015 15:51:05 +0200 Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Mon, Oct 12, 2015 at 01:27:35PM +0300, Peter Mamonov wrote: > > On Fri, 9 Oct 2015 18:11:44 +0200 > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > On Fri, Oct 09, 2015 at 03:40:37PM +0300, Peter Mamonov wrote: > > > > On Fri, 9 Oct 2015 10:06:24 +0200 > > > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > > > > > Hi Peter, > > > > > > > > > > On Wed, Oct 07, 2015 at 09:03:56PM +0300, Peter Mamonov wrote: > > > > > > Deleted pieces of code detect MBR-containig device as a FAT-type > > > > > > device, if it's first partition contains a FAT filesystem. So, > > > > > > one can mount the first partition of a hard drive containing > > > > > > FAT FS using the following command: barebox: > > > > > > mount /dev/ata0.0 /mnt/0 as well as this one: > > > > > > barebox: mount /dev/ata0 /mnt/1 > > > > > > Both commands mount the same FS. > > > > > > > > > > > > This behaviour causes automount (mount -a) to mount FAT FS > > > > > > on a first partition twice: > > > > > > barebox: mount > > > > > > none on / type ramfs > > > > > > none on /dev type devfs > > > > > > /dev/ata0 on /mnt/ata0 type fat > > > > > > /dev/ata0.0 on /mnt/ata0.0 type fat > > > > > > /dev/ata0.1 on /mnt/ata0.1 type ext4 > > > > > > > > > > This is_fat_or_mbr mechanism never worked very well and had funny > > > > > side effects. Would be nice to get rid of it. > > > > > Simply removing this option is not a solution though, we have to > > > > > find a proper way to keep the current feature and make it more > > > > > sane. > > > > > > > > Ok, the patch comment is misleading a bit. I do not propose to get > > > > rid of the is_fat_or_mbr() completely. However, I do not see the > > > > point to check for a FAT FS, after the device was correctly > > > > detected as an MBR-type device: > > > > > > > > enum filetype file_name_detect_type(const char *filename) > > > > ... > > > > type = file_detect_type(buf, ret); > > > > > > > > if (type == filetype_mbr) { > > > > /* > > > > * Get the first partition start sector > > > > * and check for FAT in it > > > > */ > > > > is_fat_or_mbr(buf, &bootsec); > > > > ret = lseek(fd, (bootsec) * 512, SEEK_SET); > > > > if (ret < 0) > > > > goto err_out; > > > > ret = read(fd, buf, 512); > > > > if (ret < 0) > > > > goto err_out; > > > > type = is_fat_or_mbr((u8 *)buf, NULL); > > > > } > > > > > > > > > > > > The deleted code snippet was introduced by this patch: > > > > > > > > commit 010ee209b75c5732ae4144e3ee9ce14158193c1f > > > > Author: Franck Jullien <franck.jullien@gmail.com> > > > > Date: Wed Sep 19 13:09:01 2012 +0200 > > > > > > > > filetype: Improve FAT detection > > > > > > > > We may have some disk with MBR as a first sector. In this case, > > > > the current FAT check returns an error. However, the FAT sector > > > > exist and the MBR can tell us where it is. > > > > > > > > This patch add to file_name_detect_type function the ability to > > > > find the FAT boot sector on the first sector of the first partition > > > > in case it is not on sector 0. > > > > > > > > It also introduce is_fat_or_mbr to check if a buffer is a FAT > > > > boot or MBR sector > > > > > > > > Signed-off-by: Franck Jullien <franck.jullien@gmail.com> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > > > > > According to the patch message it was introduced to workaround FAT > > > > detection. However, after deletion of the code I'm still able to > > > > detect and mount FAT-containig partiotions. > > > > > > But can you mount /dev/disk0 if this disk contains a partition table > > > and the FAT is on /dev/disk0.0? > > > > No. This is actually the purpose of my patch, since I don't want > > "mount -a" to mount the same partition (FAT on /dev/disk0.0) twice. > > I know, and this is valid. It just conflicts with what Franck wants. He > just wants to mount a USB device without having to know if the FAT is on > the raw device or on the first partition. > > > > > > This is what the patch is about. The > > > problem the patches solved is that when you plug in a USB drive then > > > you don't know whether a FAT is directly on the device or if the > > > device is partitioned. You want to be able to mount both ways with > > > the same command, so no matter if the FAT is on /dev/disk0 > > > or /dev/disk0.0 you can mount both using /dev/disk0. > > > > Ok. So what is the preferred way to prevent "mount -a" from mounting > > /dev/disk0 and /dev/disk0.0 at the same time? > > Sorry, I do not have a solution currently. I'll have a look into it. Can we just add a .config option for disabling "Frank mode"? -- Best regards, Antony Pavlov _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-12 14:21 ` Antony Pavlov @ 2015-10-12 14:37 ` Franck Jullien 2015-10-13 8:32 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Franck Jullien @ 2015-10-12 14:37 UTC (permalink / raw) To: Antony Pavlov; +Cc: barebox, Peter Mamonov 2015-10-12 16:21 GMT+02:00 Antony Pavlov <antonynpavlov@gmail.com>: > On Mon, 12 Oct 2015 15:51:05 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > >> On Mon, Oct 12, 2015 at 01:27:35PM +0300, Peter Mamonov wrote: >> > On Fri, 9 Oct 2015 18:11:44 +0200 >> > Sascha Hauer <s.hauer@pengutronix.de> wrote: >> > >> > > On Fri, Oct 09, 2015 at 03:40:37PM +0300, Peter Mamonov wrote: >> > > > On Fri, 9 Oct 2015 10:06:24 +0200 >> > > > Sascha Hauer <s.hauer@pengutronix.de> wrote: >> > > > >> > > > > Hi Peter, >> > > > > >> > > > > On Wed, Oct 07, 2015 at 09:03:56PM +0300, Peter Mamonov wrote: >> > > > > > Deleted pieces of code detect MBR-containig device as a FAT-type >> > > > > > device, if it's first partition contains a FAT filesystem. So, >> > > > > > one can mount the first partition of a hard drive containing >> > > > > > FAT FS using the following command: barebox: >> > > > > > mount /dev/ata0.0 /mnt/0 as well as this one: >> > > > > > barebox: mount /dev/ata0 /mnt/1 >> > > > > > Both commands mount the same FS. >> > > > > > >> > > > > > This behaviour causes automount (mount -a) to mount FAT FS >> > > > > > on a first partition twice: >> > > > > > barebox: mount >> > > > > > none on / type ramfs >> > > > > > none on /dev type devfs >> > > > > > /dev/ata0 on /mnt/ata0 type fat >> > > > > > /dev/ata0.0 on /mnt/ata0.0 type fat >> > > > > > /dev/ata0.1 on /mnt/ata0.1 type ext4 >> > > > > >> > > > > This is_fat_or_mbr mechanism never worked very well and had funny >> > > > > side effects. Would be nice to get rid of it. >> > > > > Simply removing this option is not a solution though, we have to >> > > > > find a proper way to keep the current feature and make it more >> > > > > sane. >> > > > >> > > > Ok, the patch comment is misleading a bit. I do not propose to get >> > > > rid of the is_fat_or_mbr() completely. However, I do not see the >> > > > point to check for a FAT FS, after the device was correctly >> > > > detected as an MBR-type device: >> > > > >> > > > enum filetype file_name_detect_type(const char *filename) >> > > > ... >> > > > type = file_detect_type(buf, ret); >> > > > >> > > > if (type == filetype_mbr) { >> > > > /* >> > > > * Get the first partition start sector >> > > > * and check for FAT in it >> > > > */ >> > > > is_fat_or_mbr(buf, &bootsec); >> > > > ret = lseek(fd, (bootsec) * 512, SEEK_SET); >> > > > if (ret < 0) >> > > > goto err_out; >> > > > ret = read(fd, buf, 512); >> > > > if (ret < 0) >> > > > goto err_out; >> > > > type = is_fat_or_mbr((u8 *)buf, NULL); >> > > > } >> > > > >> > > > >> > > > The deleted code snippet was introduced by this patch: >> > > > >> > > > commit 010ee209b75c5732ae4144e3ee9ce14158193c1f >> > > > Author: Franck Jullien <franck.jullien@gmail.com> >> > > > Date: Wed Sep 19 13:09:01 2012 +0200 >> > > > >> > > > filetype: Improve FAT detection >> > > > >> > > > We may have some disk with MBR as a first sector. In this case, >> > > > the current FAT check returns an error. However, the FAT sector >> > > > exist and the MBR can tell us where it is. >> > > > >> > > > This patch add to file_name_detect_type function the ability to >> > > > find the FAT boot sector on the first sector of the first partition >> > > > in case it is not on sector 0. >> > > > >> > > > It also introduce is_fat_or_mbr to check if a buffer is a FAT >> > > > boot or MBR sector >> > > > >> > > > Signed-off-by: Franck Jullien <franck.jullien@gmail.com> >> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> > > > >> > > > According to the patch message it was introduced to workaround FAT >> > > > detection. However, after deletion of the code I'm still able to >> > > > detect and mount FAT-containig partiotions. >> > > >> > > But can you mount /dev/disk0 if this disk contains a partition table >> > > and the FAT is on /dev/disk0.0? >> > >> > No. This is actually the purpose of my patch, since I don't want >> > "mount -a" to mount the same partition (FAT on /dev/disk0.0) twice. >> >> I know, and this is valid. It just conflicts with what Franck wants. He >> just wants to mount a USB device without having to know if the FAT is on >> the raw device or on the first partition. >> >> > >> > > This is what the patch is about. The >> > > problem the patches solved is that when you plug in a USB drive then >> > > you don't know whether a FAT is directly on the device or if the >> > > device is partitioned. You want to be able to mount both ways with >> > > the same command, so no matter if the FAT is on /dev/disk0 >> > > or /dev/disk0.0 you can mount both using /dev/disk0. >> > >> > Ok. So what is the preferred way to prevent "mount -a" from mounting >> > /dev/disk0 and /dev/disk0.0 at the same time? >> >> Sorry, I do not have a solution currently. I'll have a look into it. > > Can we just add a .config option for disabling "Frank mode"? > > -- > Best regards, > Antony Pavlov > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox This is not important for me. I faced this situation when I was playing with SD cards controllers. However, if I had a problem it may arise for someone else. Feel free to remove this detection or, as Antony suggested, add a config option. Franck. _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] common: filetype: is_fat_or_mbr() considered harmful 2015-10-12 14:37 ` Franck Jullien @ 2015-10-13 8:32 ` Sascha Hauer 0 siblings, 0 replies; 9+ messages in thread From: Sascha Hauer @ 2015-10-13 8:32 UTC (permalink / raw) To: Franck Jullien; +Cc: barebox, Peter Mamonov On Mon, Oct 12, 2015 at 04:37:18PM +0200, Franck Jullien wrote: > >> > > > According to the patch message it was introduced to workaround FAT > >> > > > detection. However, after deletion of the code I'm still able to > >> > > > detect and mount FAT-containig partiotions. > >> > > > >> > > But can you mount /dev/disk0 if this disk contains a partition table > >> > > and the FAT is on /dev/disk0.0? > >> > > >> > No. This is actually the purpose of my patch, since I don't want > >> > "mount -a" to mount the same partition (FAT on /dev/disk0.0) twice. > >> > >> I know, and this is valid. It just conflicts with what Franck wants. He > >> just wants to mount a USB device without having to know if the FAT is on > >> the raw device or on the first partition. > >> > >> > > >> > > This is what the patch is about. The > >> > > problem the patches solved is that when you plug in a USB drive then > >> > > you don't know whether a FAT is directly on the device or if the > >> > > device is partitioned. You want to be able to mount both ways with > >> > > the same command, so no matter if the FAT is on /dev/disk0 > >> > > or /dev/disk0.0 you can mount both using /dev/disk0. > >> > > >> > Ok. So what is the preferred way to prevent "mount -a" from mounting > >> > /dev/disk0 and /dev/disk0.0 at the same time? > >> > >> Sorry, I do not have a solution currently. I'll have a look into it. > > > > Can we just add a .config option for disabling "Frank mode"? > > > > -- > > Best regards, > > Antony Pavlov > > > > _______________________________________________ > > barebox mailing list > > barebox@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/barebox > > This is not important for me. > I faced this situation when I was playing with SD cards controllers. > > However, if I had a problem it may arise for someone else. > > Feel free to remove this detection or, as Antony suggested, add a config option. The desired behaviour can be reached with a: mount /dev/disk0 /fat || mount /dev/disk0.0 /fat || echo "Mounting failed" So I tend to say that we should remove the is_fat_or_mbr detection and everything around it. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-13 8:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-07 18:03 [RFC] common: filetype: is_fat_or_mbr() considered harmful Peter Mamonov 2015-10-09 8:06 ` Sascha Hauer 2015-10-09 12:40 ` Peter Mamonov 2015-10-09 16:11 ` Sascha Hauer 2015-10-12 10:27 ` Peter Mamonov 2015-10-12 13:51 ` Sascha Hauer 2015-10-12 14:21 ` Antony Pavlov 2015-10-12 14:37 ` Franck Jullien 2015-10-13 8:32 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox