Re: [Libguestfs] [synnefo-devel] Re: [PATCH 1/1] inspect: Fix a bug in the *BSD root detection
LGTM
Another thing concerning the disklabel partitions is that
list_filesystems() will print both /dev/sda1 and /dev/sda5 as ufs file
systems. I don't know if you care to change this. The fact is that both
are mountable.
P.S. I wish the kernel folks would stop treating the disklabel
partitions as logical partitions. Logical partitions are sequential,
there are no gaps in the numbering. On the other hand disklabel may
have gaps like the MBR partitions. Having defined partitions 'a', 'b'
and 'c' is completely different than having defined partitions 'a',
'c'
and 'd'. The kernel in both cases will show /dev/sda5 and /dev/sda6. I
wonder if they could get convinced to change that...
On 28/11/14 16:59, Richard W.M. Jones wrote:
On Fri, Nov 28, 2014 at 03:42:58PM +0100, Pino Toscano wrote:
> On Friday 28 November 2014 14:31:01 Richard W.M. Jones wrote:
>> How about the attached patch? It's basically the same as your patch
>> but I moved the code between files and tidied up some whitespace
>> issues.
> Present in both the patches:
>
>> +/* On *BSD systems, sometimes /dev/sda[1234] is a shadow of the real root
>> + * filesystem that is probably /dev/sda5
>> + * (see: http://www.freebsd.org/doc/handbook/disk-organization.html)
>> + */
>> +static void
>> +check_for_duplicated_bsd_root (guestfs_h *g)
>> +{
>> + size_t i;
>> + bool is_primary, is_bsd;
>> + struct inspect_fs *fs, *bsd_primary = NULL;
>> +
>> + for (i = 0; i < g->nr_fses; ++i) {
>> + fs = &g->fses[i];
>> +
>> + is_primary = match (g, fs->mountable, re_primary_partition);
>> + is_bsd =
>> + fs->type == OS_TYPE_FREEBSD ||
>> + fs->type == OS_TYPE_NETBSD ||
>> + fs->type == OS_TYPE_OPENBSD;
>> +
>> + if (fs->is_root && is_primary && is_bsd) {
>> + bsd_primary = fs;
>> + continue;
>> + }
> This will run the regexp matching for every filesystem found; what
> about inlining the match call as last part of the if, like:
>
> is_bsd =
> fs->type == OS_TYPE_FREEBSD ||
> fs->type == OS_TYPE_NETBSD ||
> fs->type == OS_TYPE_OPENBSD;
>
> if (fs->is_root && is_bsd && is_primary
> && match (g, fs->mountable, re_primary_partition)) {
> bsd_primary = fs;
> continue;
> }
>
> This way it is done only for *BSD filesystems, and is_primary is
> used only in that if anyway.
>
> Also, is_bsd and fs could be declared just inside the for, as they are
> not needed outside of it.
Good points.
See updated patch below.
Rich.