Libguestfs - Nov 2014 - Re: [PATCH 1/1] inspect: Fix a bug in the *BSD root detection

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.

-- 
Pino Toscano

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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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.
>