Re: [OE-core] [PATCH v5 01/10] wic: re-implement sector-size support

[Paul Barker <paul@pbarker.dev>]

[-- Attachment #1: Type: text/plain, Size: 7264 bytes --]

On Tue, 2026-02-24 at 13:40 -0500, Trevor Woerner wrote:
> On Tue 2026-02-24 @ 09:52:55 AM, Paul Barker wrote:
> > On Mon, 2026-02-23 at 16:49 -0500, Trevor Woerner via
> > lists.openembedded.org wrote:
> > > @@ -340,11 +331,15 @@ class Disk:
> > >              raise WicError("Partition %s is not in the image" % pnum)
> > >          part = self.partitions[pnum]
> > >          # check if fstype is supported
> > > +        # NOTE: parted is unable to identify dos-type partitions with a 4k sector-size
> > > +        #       if the type is empty and a non-default sector size is used, assume 'fat'
> > > +        #       fdisk identifies them without issue
> > > +        part_fstype = part.fstype if part.fstype or self.sector_size == 512 else 'fat'
> > 
> > This is difficult to parse. What's the result if part.fstype is None but
> > self.sector_size is 512?
> 
> I had added 3 lines of comments before this line hoping it would help :-D
> 
> The crux of the problem is that wic uses parted to identify partition
> types. I found a bug in parted whereby it can't identify dos-type
> partitions when using sector-sizes that aren't 512.
> 
> Note that it is entirely possible that the partition type is not known
> or invalid; in which case parted will not be able to identify the
> partition type and part.fstype will be None. If sector-size=512 and
> parted does not know the type (None) then that is probably accurate.
> However if sector-size!=512 and parted can not identify the partition
> type then either the partition is truly invalid, or it might be type fat
> (due to the bug in parted).
> 
> I was able to use fdisk to confirm that fat-style partitions created by
> wic that have sector-size!=512 are in fact valid partitions. So you
> might wonder: why not switch wic from using parted to identify partition
> types to using fdisk? That would be a larger project, and probably
> something that wic will have to consider at some point. Or possibly try
> to create a patch for parted to correctly identify dos-type partitions
> when sector-size!=512. The problem is that the output from fdisk is not
> easy to parse, and is known to change from time to time. parted offers
> an -m option that will cause it to produce output that is very easy to
> parse whereas fdisk offers no such option, making any code that tries to
> read fdisk output more difficult to maintain.
> 
> So, for now hopefully, the above line will accept parted's assessment of
> the partition type even if it is None when sector-size=512. If
> sector-size!=512 then use parted's assessment unless it is None, in
> which case assume fat.
> 
> The above line performs this logic correctly (I'm fairly sure).
> 
> Would you prefer to see an expanded comment? Or would you prefer to see
> a ... more explicit way of writing that code?

Ok, this makes sense given parted limitations. Agreed that switching to
using fdisk is a project for another day.

I struggle to parse backwards if conditions (what Python calls
"conditional expressions") with more than one thing going on, but it
looks correct if I squint at it more so let's just go with it.

> > > @@ -638,14 +634,19 @@ class Disk:
> > >  
> > >  def wic_ls(args, native_sysroot):
> > >      """List contents of partitioned image or vfat partition."""
> > > -    disk = Disk(args.path.image, native_sysroot)
> > > +    disk = Disk(args.path.image, native_sysroot, sector_size=args.sector_size)
> > >      if not args.path.part:
> > >          if disk.partitions:
> > >              print('Num     Start        End          Size      Fstype')
> > >              for part in disk.partitions.values():
> > > +                # size values are in bytes from parted; convert to sectors if a custom sector size was requested
> > > +                display_size = part.size
> > > +                if args.sector_size and args.sector_size != disk._lsector_size:
> > > +                    display_size = part.size // args.sector_size
> > >                  print("{:2d}  {:12d} {:12d} {:12d}  {}".format(\
> > > -                          part.pnum, part.start, part.end,
> > > -                          part.size, part.fstype))
> > > +                          part.pnum, part.start // args.sector_size,
> > > +                          part.end // args.sector_size,
> > > +                          display_size, part.fstype))
> > 
> > This looks like it could confuse users - is it always clear to the user
> > whether the output is in bytes or in sectors? Why not leave this always
> > printing the size in bytes so that there's no confusion?
> 
> I was following fdisk's lead here. When you use fdisk to display an
> image's partition table, the numbers are expressed in sector-sized
> values, and since this output looks a lot like what fdisk would print
> out, I was copying its behaviour.

Ok, if we're copying behaviour of an existing tool that makes sense. I
want to avoid surprising users.

> > > diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
> > > index 8fed686e903e..df6e3271649b 100644
> > > --- a/scripts/lib/wic/partition.py
> > > +++ b/scripts/lib/wic/partition.py
> > > @@ -65,6 +65,50 @@ class Partition():
> > >  
> > >          self.lineno = lineno
> > >          self.source_file = ""
> > > +        self.sector_size = 512
> > > +
> > > +    def _mkdosfs_extraopts(self):
> > > +        """
> > > +        Build mkdosfs extra options ensuring the CLI sector size is applied.
> > > +        """
> > > +        extraopts = self.mkfs_extraopts or ''
> > > +        tokens = []
> > > +        skip_next = False
> > > +        for tok in extraopts.split():
> > > +            if skip_next:
> > > +                skip_next = False
> > > +                continue
> > > +            if tok == '-S':
> > > +                skip_next = True
> > > +                continue
> > > +            if tok.startswith('-S'):
> > > +                continue
> > > +            tokens.append(tok)
> > > +        tokens.extend(['-S', str(self.sector_size)])
> > > +        return ' '.join(tokens).strip()
> > 
> > This section needs a comment to explain what it's doing.
> 
> Both _mkdosfs_extraopts() and _mkfs_ext_extraopts() are performing,
> essentially, the same thing using very similar code. But you
> only made this comment for _mkfs_ext_extraopts(), does that mean
> _mkdosfs_extraopts() is understandable but _mkfs_ext_extraopts() is not?
> 
> Both of them have docstrings that explain their function... albeit
> rather tersely. Would you like to see these docstrings expanded? I'd be
> happy to expand the docstring for one or both of them.

_mkfs_ext_extraopts has the comment:

    # Only add an explicit block size when a non-default sector size is requested.

But looking again, that doesn't quite explain well enough. A comment
in both place that this is ignoring any sector size argument already
specified and replacing it with self.sector_size is probably good to
have. It begs the question - is that safe to do? Or should we throw an
error if self.mkfs_extraopts includes an option to set the sector size?

Best regards,

-- 
Paul Barker


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 252 bytes --]