Re: [PATCH 4/6] tools/libfsimage: Add an fdopen() interface to libfsimage

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 22/11/2023 22:29, Andrew Cooper wrote:
> On 06/11/2023 3:05 pm, Alejandro Vallejo wrote:
> > diff --git a/tools/libfsimage/common/fsimage_priv.h 
> > b/tools/libfsimage/common/fsimage_priv.h
> > index 2274403557..779e433b37 100644
> > --- a/tools/libfsimage/common/fsimage_priv.h
> > +++ b/tools/libfsimage/common/fsimage_priv.h
> > @@ -29,6 +29,7 @@ extern C {
> >   #endif
> >   
> >   #include <sys/types.h>
> > +#include <stdbool.h>
> >   
> >   #include "xenfsimage.h"
> >   #include "xenfsimage_plugin.h"
> > @@ -54,7 +55,7 @@ struct fsi_file {
> >         void *ff_data;
> >   };
> >   
> > -int find_plugin(fsi_t *, const char *, const char *);
> > +int find_plugin(fsi_t *, const char *);
> >   
> >   #ifdef __cplusplus
> >   };
> These are the only two hunks in this file.  Is the stdbool include stale?
>
> It seems to compile fine with it removed.
I think it's leftover from code I needed initially and then didn't 
anymore. Safe to get rid of.
> > diff --git a/tools/libfsimage/ext2fs-lib/ext2fs-lib.c 
> > b/tools/libfsimage/ext2fs-lib/ext2fs-lib.c
> > index 864a15b349..9f07ea288f 100644
> > --- a/tools/libfsimage/ext2fs-lib/ext2fs-lib.c
> > +++ b/tools/libfsimage/ext2fs-lib/ext2fs-lib.c
> > @@ -25,15 +25,25 @@
> >   #include INCLUDE_EXTFS_H
> >   #include <errno.h>
> >   #include <inttypes.h>
> > +#include <stdio.h>
> >   
> >   static int
> > -ext2lib_mount(fsi_t *fsi, const char *name, const char *options)
> > +ext2lib_mount(fsi_t *fsi, const char *options)
> >   {
> >         int err;
> >         char opts[30] = "";
> >         ext2_filsys *fs;
> >         uint64_t offset = fsip_fs_offset(fsi);
> >   
> > +	/*
> > +        * We must choose unixfd_io_manager rather than unix_io_manager in
> > +        * order for the library to accept fd strings instead of paths. It
> > +        * still means we must pass a string representing an fd rather than
> > +        * an int, but at least this way we don't need to pass paths around
> > +        */
> > +       char name[32] = {0};
> For an int?  12 will do fine including a terminator, and practical
> system limits leave it far smaller than that generally.
Maybe. I admit optimising that line went pretty low on the list of 
things I cared terribly about. I'm happy to reduce it, but it is 
inconsequential.
> Given that it is guaranteed long enough, you don't need to zero it just
> to have snprintf() write a well-formed string in.
>
> ~Andrew
For wellformed fd's that's true. Through bugs or malice that may not be 
the case. I'm happier knowing at least the last zero remains.
Cheers,
Alejandro