[syslinux] [PATCH 03/12] core: initial implementation of fopen with multidisk acess
Matt Fleming
matt at console-pimps.org
Mon Oct 15 06:42:00 PDT 2012
On Mon, 2012-08-20 at 05:16 -0300, Andre Ericson wrote:
> An initial implementation of fopen. Should work for GPT/MBR.
>
> cache still needs to be modified as syslinux will break after opening
> file.
>
> Signed-off-by: Andre Ericson <de.ericson at gmail.com>
> Signed-off-by: Paulo Alcantara <pcacjr at zytor.com>
> ---
> com32/elflink/ldlinux/execute.c | 2 +-
> com32/modules/Makefile | 2 +-
> com32/modules/fopen_test.c | 30 ++
> core/conio.c | 2 +-
> core/disk.c | 2 +-
> core/fs/chdir.c | 54 ++-
> core/fs/fs.c | 209 +++++++++-
> core/fs/lib/chdir.c | 5 +-
> core/fs/pxe/pxe.c | 2 +-
> core/fs/readdir.c | 2 +-
> core/include/fs.h | 11 +-
> core/include/multidisk.h | 8 +
> core/include/partiter.h | 105 ++++++
> core/multidisk.c | 52 +++
> core/partiter.c | 816 ++++++++++++++++++++++++++++++++++++++++
> 15 files changed, 1271 insertions(+), 31 deletions(-)
> create mode 100644 com32/modules/fopen_test.c
> create mode 100644 core/include/multidisk.h
> create mode 100644 core/include/partiter.h
> create mode 100644 core/multidisk.c
> create mode 100644 core/partiter.c
You are introducing lots of new code here. Your commit log needs to be
much, much more verbose. I've asked some questions below, the answers to
which I think should be mentioned in the commit log.
Also, you shouldn't be asking for commits to be pulled that break
things. I realise that by the end of the series things will work, but if
someone is trying to do a git bisect once this series is merged and they
build with this commit as HEAD, their system will not function
correctly. You should gradually introduce new functionality, making sure
that at every stage things compile and no regressions are introduced.
Splitting new functionality across two or more patches is asking for
trouble.
> diff --git a/com32/elflink/ldlinux/execute.c b/com32/elflink/ldlinux/execute.c
> index e7969c2..ff8393d 100644
> --- a/com32/elflink/ldlinux/execute.c
> +++ b/com32/elflink/ldlinux/execute.c
> @@ -111,7 +111,7 @@ void execute(const char *cmdline, uint32_t type)
>
> /* If we got anything on the command line, do a chdir */
> if (*args)
> - mangle_name(config_cwd, args);
> + mangle_name(config_cwd, args, NULL);
>
> start_ldlinux(argv);
> } else if (type == IMAGE_TYPE_LOCALBOOT) {
This patch would have been smaller if you'd introduced two new functions
that take the fs_info * argument, say, __mangle_name() and
__searchdir(). Or perhaps fs_mangle_name() and fs_searchdir() and then
just make mangle_name() call fs_mangle_name(cwd, args, NULL).
What does 'this_fs' actually mean now? The filesystem on which Syslinux
is installed? The filesystem that contains our current working
directory? The answer to this question should be in the commit log.
> diff --git a/com32/modules/Makefile b/com32/modules/Makefile
> index 8f5b769..7c396ed 100644
> --- a/com32/modules/Makefile
> +++ b/com32/modules/Makefile
> @@ -27,7 +27,7 @@ MODULES = config.c32 ethersel.c32 dmitest.c32 cpuidtest.c32 \
> meminfo.c32 sdi.c32 sanboot.c32 ifcpu64.c32 vesainfo.c32 \
> kbdmap.c32 cmd.c32 vpdtest.c32 host.c32 ls.c32 gpxecmd.c32 \
> ifcpu.c32 cpuid.c32 cat.c32 pwd.c32 ifplop.c32 zzjson.c32 \
> - whichsys.c32 hello.c32 pxechn.c32
> + whichsys.c32 hello.c32 pxechn.c32 fopen_test.c32
>
> TESTFILES =
>
> diff --git a/com32/modules/fopen_test.c b/com32/modules/fopen_test.c
> new file mode 100644
> index 0000000..a6a2ec1
> --- /dev/null
> +++ b/com32/modules/fopen_test.c
> @@ -0,0 +1,30 @@
> +/*
> + * fopen_test.c - A simple ELF module to test fopen on a different partition.
> + *
> + * Created on: Jun 30, 2012
> + * Author: Andre Ericson <de.ericson at gmail.com>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#define FILENAME "(0 2):/andre/hello_mate"
> +
> +int main(int argc __unused, char **argv __unused)
> +{
> + /* should have this syntax: (hd part):/path/to/file */
> + FILE *f = fopen(FILENAME, "rt");
> + char buff[50];
> + int i = 0;
> +
> + if (!f)
> + printf("File not found.\n"
> + "For multidisk files use (hd part):/path/to/file\n");
> + else {
> + while ((buff[i++] = fgetc(f)) && i < 50);
> + buff[i < 49 ?i : 49] = '\0';
> +
> + printf("read %s\n", buff);
> + }
> +
> + return 0;
> +}
> diff --git a/core/conio.c b/core/conio.c
> index 421dabf..c94caa1 100644
> --- a/core/conio.c
> +++ b/core/conio.c
> @@ -445,7 +445,7 @@ static void msg_viewimage(void)
>
> *VGAFilePtr = '\0'; /* Zero-terminate filename */
>
> - mangle_name(VGAFileMBuf, VGAFileBuf);
> + mangle_name(VGAFileMBuf, VGAFileBuf, NULL);
> f = fopen(VGAFileMBuf, "r");
> if (!f) {
> /* Not there */
> diff --git a/core/disk.c b/core/disk.c
> index 5e3ce61..9280c83 100644
> --- a/core/disk.c
> +++ b/core/disk.c
> @@ -74,7 +74,7 @@ int disk_int13_retry(const com32sys_t * inreg, com32sys_t * outreg)
> */
> int disk_get_params(int disk, struct disk_info *const diskinfo)
> {
> - static com32sys_t inreg, outreg;
> + com32sys_t inreg, outreg;
> struct disk_ebios_eparam *eparam = NULL;
>
> memset(diskinfo, 0, sizeof *diskinfo);
Why make 'inreg' and 'outreg' non-static?
> diff --git a/core/fs/chdir.c b/core/fs/chdir.c
> index 903cabc..e42f0dd 100644
> --- a/core/fs/chdir.c
> +++ b/core/fs/chdir.c
> @@ -65,7 +65,7 @@ size_t realpath(char *dst, const char *src, size_t bufsize)
> if (this_fs->fs_ops->realpath) {
> s = this_fs->fs_ops->realpath(this_fs, dst, src, bufsize);
> } else {
> - rv = searchdir(src);
> + rv = searchdir(src, NULL);
> if (rv < 0) {
> dprintf("realpath: searchpath failure\n");
> return -1;
> @@ -97,7 +97,7 @@ int chdir(const char *src)
> return this_fs->fs_ops->chdir(this_fs, src);
>
> /* Otherwise it is a "conventional filesystem" */
> - rv = searchdir(src);
> + rv = searchdir(src, NULL);
> if (rv < 0)
> return rv;
>
> @@ -129,3 +129,53 @@ int chdir(const char *src)
>
> return 0;
> }
> +
> +/* won't merge this and chdir(const char*) to not break compatibility
> + * with unistd.h */
> +int multidisk_chdir(const char *src, struct fs_info *fp)
> +{
> + int rv;
> + struct file *file;
> + char cwd_buf[CURRENTDIR_MAX];
> + size_t s;
> +
> + dprintf("chdir: from %s (inode %p) add %s\n",
> + fp->cwd_name, fp->cwd, src);
> +
> + if (fp->fs_ops->chdir)
> + return fp->fs_ops->chdir(fp, src);
> +
> + /* Otherwise it is a "conventional filesystem" */
> + rv = searchdir(src, fp);
> +
> + if (rv < 0)
> + return rv;
> +
> + file = handle_to_file(rv);
> + if (file->inode->mode != DT_DIR) {
> + _close_file(file);
> + return -1;
> + }
> +
> + put_inode(fp->cwd);
> + fp->cwd = get_inode(file->inode);
> + _close_file(file);
> +
> + /* Save the current working directory */
> + s = generic_inode_to_path(fp->cwd, cwd_buf, CURRENTDIR_MAX-1);
> +
> + /* Make sure the cwd_name ends in a slash, it's supposed to be a prefix */
> + if (s < 1 || cwd_buf[s-1] != '/')
> + cwd_buf[s++] = '/';
> +
> + if (s >= CURRENTDIR_MAX)
> + s = CURRENTDIR_MAX - 1;
> +
> + cwd_buf[s++] = '\0';
> + memcpy(fp->cwd_name, cwd_buf, s);
> +
> + dprintf("chdir: final %s (inode %p)\n",
> + fp->cwd_name, fp->cwd);
> +
> + return 0;
> +}
You've got the right idea here - introducing a new function, but there's
nothing inherently 'multidisk' about this function. fs_chdir() or
something would have been a better name.
[...]
> +struct fs_info *get_fs_info(uint8_t hdd, uint8_t partition)
> +{
> + struct fs_info fs;
> + uint8_t disk_devno, disk_cdrom;
> + sector_t disk_offset;
> + uint16_t disk_heads, disk_sectors, maxtransfer;
> + struct part_iter *iter = NULL;
> + uint8_t buff[512];
> + struct disk_info diskinfo;
> + const struct fs_ops **ops;
> + int blk_shift = -1;
> + struct device *dev = NULL;
> +
> + if (fs_array[hdd][partition - 1])
> + return fs_array[hdd][partition - 1];
> +
> + disk_devno = 0x80 + hdd;
> +
> + /* For some unknown reason without this call to a "nop" function
> + * an infinite loop happens, must investigate later
> + * TODO: fix this */
> + do_magic(buff);
> +
> + if (find_partition(&iter, disk_devno, partition)) {
> + printf("Failed to get partition\n");
> + return NULL;
> + }
> + else
> + dprintf("part_offset 0x%llx\n", iter->start_lba);
> +
> + disk_offset = iter->start_lba;
> +
> + if (disk_get_params(disk_devno, &diskinfo))
> + return NULL;
> +
> + disk_heads = diskinfo.head;
> + disk_sectors = diskinfo.spt;
> +
> + /* force those values for now
> + * TODO: fix this */
> + disk_cdrom = 0;
> + maxtransfer = 127;
> +
> + ops = fs_ops_array;
> +
> +
> + /* Default name for the root directory */
> + fs.cwd_name[0] = '/';
> +
> + while ((blk_shift < 0) && *ops) {
> + /* set up the fs stucture */
> + fs.fs_ops = *ops;
> +
> + /*
> + * This boldly assumes that we don't mix FS_NODEV filesystems
> + * with FS_DEV filesystems...
> + */
> + if (fs.fs_ops->fs_flags & FS_NODEV) {
> + fs.fs_dev = NULL;
> + } else {
> + if (!dev)
> + dev = device_init(disk_devno, disk_cdrom, disk_offset,
> + disk_heads, disk_sectors, maxtransfer);
> + fs.fs_dev = dev;
> + }
> + /* invoke the fs-specific init code */
> + blk_shift = fs.fs_ops->fs_init(&fs);
> + ops++;
> + }
> + if (blk_shift < 0) {
> + printf("No valid file system found!\n");
> + while (1)
> + ;
This seems pretty extreme. Printing the warning and bailing would seem
more user friendly, rather than hanging the machine.
> + }
> + fs_array[hdd][partition - 1] = &fs;
> +
> + /* initialize the cache */
> + if (fs.fs_dev && fs.fs_dev->cache_data)
> + cache_init(fs.fs_dev, blk_shift);
> +
> + /* start out in the root directory */
> + if (fs.fs_ops->iget_root) {
> + fs.root = fs.fs_ops->iget_root(&fs);
> + fs.cwd = get_inode(fs.root);
> + }
> +
> + if (fs.fs_ops->chdir_start) {
> + if (fs.fs_ops->chdir_start(fs_array[hdd][partition -1]) < 0)
> + printf("Failed to chdir to start directory\n");
> + }
Shouldn't we be returning some kind of error code here if we fail to do
the chdir?
> + return fs_array[hdd][partition - 1];
> +}
> +
> +
> void pm_searchdir(com32sys_t *regs)
> {
> char *name = MK_PTR(regs->ds, regs->edi.w[0]);
> int rv;
>
> - rv = searchdir(name);
> + rv = searchdir(name, NULL);
> if (rv < 0) {
> regs->esi.w[0] = 0;
> regs->eax.l = 0;
> @@ -217,7 +330,7 @@ void pm_searchdir(com32sys_t *regs)
> }
> }
>
> -int searchdir(const char *name)
> +int searchdir(const char *name, struct fs_info *fp)
> {
> struct inode *inode = NULL;
> struct inode *parent = NULL;
> @@ -228,10 +341,13 @@ int searchdir(const char *name)
>
> dprintf("searchdir: %s root: %p cwd: %p\n",
> name, this_fs->root, this_fs->cwd);
> + if (!fp)
> + fp = this_fs;
>
> if (!(file = alloc_file()))
> goto err_no_close;
> - file->fs = this_fs;
> +
> + file->fs = fp;
>
> /* if we have ->searchdir method, call it */
> if (file->fs->fs_ops->searchdir) {
> @@ -245,7 +361,7 @@ int searchdir(const char *name)
>
> /* else, try the generic-path-lookup method */
>
> - parent = get_inode(this_fs->cwd);
> + parent = get_inode(fp->cwd);
> p = pathbuf = strdup(name);
> if (!pathbuf)
> goto err;
> @@ -254,7 +370,7 @@ int searchdir(const char *name)
> got_link:
> if (*p == '/') {
> put_inode(parent);
> - parent = get_inode(this_fs->root);
> + parent = get_inode(fp->root);
> }
>
> do {
> @@ -285,8 +401,8 @@ int searchdir(const char *name)
> }
> }
> } else if (part[0] != '.' || part[1] != '\0') {
> - inode = this_fs->fs_ops->iget(part, parent);
> - if (!inode)
> + inode = fp->fs_ops->iget(part, parent);
> + if (!inode)
Weird indentation?
> goto err;
> if (inode->mode == DT_LNK) {
> char *linkbuf, *q;
> @@ -294,7 +410,7 @@ int searchdir(const char *name)
> int total_len = inode->size + name_len + 2;
> int link_len;
>
> - if (!this_fs->fs_ops->readlink ||
> + if (!fp->fs_ops->readlink ||
> --symlink_count == 0 || /* limit check */
> total_len > MAX_SYMLINK_BUF)
> goto err;
> @@ -303,7 +419,7 @@ int searchdir(const char *name)
> if (!linkbuf)
> goto err;
>
> - link_len = this_fs->fs_ops->readlink(inode, linkbuf);
> + link_len = fp->fs_ops->readlink(inode, linkbuf);
> if (link_len <= 0) {
> free(linkbuf);
> goto err;
> @@ -373,11 +489,70 @@ int open_file(const char *name, struct com32_filedata *filedata)
> int rv;
> struct file *file;
> char mangled_name[FILENAME_MAX];
> + uint8_t hdd = 0;
> + uint8_t partition = 0;
> + char buff[4];
> +
> + if (name[0] == '(') {
> + char relative_name[FILENAME_MAX];
> + const char *c = name;
> + int i = 0;
> + int mult = 1;
> +
> + /* get hdd number */
> + for (++c; *c != ' '; ++c)
> + buff[i++] = *c;
> + buff[i] = '\0';
> +
You need to check the bounds of 'buff' here otherwise it's possible to
overflow the buffer and write '\0' to an arbitrary part of the stack.
> + /* str to uint8_t */
> + while (i--) {
> + hdd += (buff[i] - 48) * mult;
> + mult *= 10;
> + }
Please use '0' here instead of 48.
> + /* get partition number */
> + i = 0;
> + for (++c; *c != ')'; ++c)
> + buff[i++] = *c;
> + buff[i] = '\0';
Again, needs bounds checking.
> + /* str to uint8_t */
> + mult = 1;
> + while (i--) {
> + partition += (buff[i] - 48) * mult;
> + mult *= 10;
> + }
'0' not 48.
> + i = 0;
> + /* c was on ')' jump ':' and stop at beginning of path */
> + for (c += 2; *c; c++)
> + relative_name[i++] = *c;
> + relative_name[i] = '\0';
> +
> + mangle_name(mangled_name, relative_name, get_fs_info(hdd, partition));
> +
If get_fs_info() returns NULL, you don't want to try looking up
'mangled_name' on this_fs. You need to check the return value and bail
if necessary.
[...]
> +int guid_is0(const struct guid *guid)
> +{
> + return !*(const uint64_t *)guid && !*((const uint64_t *)guid + 1);
> +}
Is there a reason this can't just be a memcmp()?
--
Matt Fleming, Intel Open Source Technology Center
More information about the Syslinux
mailing list