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
signature.asc
Description: This is a digitally signed message part
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#231904): https://lists.openembedded.org/g/openembedded-core/message/231904 Mute This Topic: https://lists.openembedded.org/mt/117965967/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
