Libguestfs - Oct 2015 - Re: [PATCH 1/2] New API: vfs_min_size

On Monday 19 October 2015 17:05:02 Maxim Perevedentsev
wrote:
> This call provides the way to get minimum size of filesystem.
> This is needed for shrinking. The return units are bytes.
> 
> ---
>  daemon/Makefile.am   |  1 +
>  daemon/daemon.h      |  1 +
>  daemon/fs-min-size.c | 46 ++++++++++++++++++++++++++++++++
>  daemon/ntfs.c        | 75
++++++++++++++++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml | 16 +++++++++++
>  po/POTFILES          |  1 +
>  src/MAX_PROC_NR      |  2 +-
>  7 files changed, 141 insertions(+), 1 deletion(-)
>  create mode 100644 daemon/fs-min-size.c
> 
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 4ea3c88..0a01a24 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -116,6 +116,7 @@ guestfsd_SOURCES = \
>  	findfs.c \
>  	fill.c \
>  	find.c \
> +	fs-min-size.c \
Hm not really convinced by the file name chosen, but I cannot find
better options. "size.c"? "fs-size.c"?
>  	fsck.c \
>  	fstrim.c \
>  	glob.c \
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 508691a..a690152 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -283,6 +283,7 @@ extern int btrfs_set_uuid_random (const char *device);
>  /*-- in ntfs.c --*/
>  extern char *ntfs_get_label (const char *device);
>  extern int ntfs_set_label (const char *device, const char *label);
> +extern int64_t ntfs_min_size (const char *device);
> 
>  /*-- in swap.c --*/
>  extern int swap_set_uuid (const char *device, const char *uuid);
> diff --git a/daemon/fs-min-size.c b/daemon/fs-min-size.c
> new file mode 100644
> index 0000000..9c107d1
> --- /dev/null
> +++ b/daemon/fs-min-size.c
> @@ -0,0 +1,46 @@
> +/* libguestfs - the guestfsd daemon
> + * Copyright (C) 2015 Maxim Perevedentsev mperevedentsev@virtuozzo.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include "daemon.h"
> +#include "actions.h"
> +
> +int64_t
> +do_vfs_min_size(const mountable_t *mountable)
I'd be slightly more verbose here, and call the action like
"vfs_minimum_size".

(also a minor style nitpick: missing space between function name and
the bracket, and in other few places in this patch)
> +{
> +  int64_t r;
> +
> +  /* How we set the label depends on the filesystem type. */
> +  CLEANUP_FREE char *vfs_type = do_vfs_type (mountable);
> +  if (vfs_type == NULL)
> +    return -1;
> +
> +  else if (STREQ (vfs_type, "ntfs"))
> +    r = ntfs_min_size (mountable->device);
> +
> +  else
> +    NOT_SUPPORTED (-1, "don't know how to get minimum size of
'%s' filesystems",
> +                   vfs_type);
The documentation might better explicitly mention that unsupported
filesystems will give ENOTSUP as errno on failure.
> +
> +  return r;
> +}
> diff --git a/daemon/ntfs.c b/daemon/ntfs.c
> index 1ead159..b9a645a 100644
> --- a/daemon/ntfs.c
> +++ b/daemon/ntfs.c
> @@ -153,6 +153,81 @@ do_ntfsresize_size (const char *device, int64_t size)
>    return do_ntfsresize (device, size, 0);
>  }
> 
> +int64_t
> +ntfs_min_size (const char *device)
> +{
> +  CLEANUP_FREE char *err = NULL, *out = NULL;
> +  CLEANUP_FREE_STRING_LIST char **lines = NULL;
> +  int r;
> +  size_t i;
> +  char *p;
> +  int64_t ret, volume_size = 0;
The scope of "ret" could be reduced to only the if below where it is
used.
> +  const char *size_pattern = "You might resize at ",
> +             *full_pattern = "Volume is full",
> +             *cluster_size_pattern = "Cluster size",
> +             *volume_size_pattern = "Current volume size:";
> +  int is_full = 0;
> +  int32_t cluster_size = 0;
> +
> +  /* FS may be marked for check, so force ntfsresize */
> +  r = command (&out, &err, str_ntfsresize, "--info",
"-ff", device, NULL);
> +
> +  lines = split_lines (out);
> +  if (lines == NULL)
> +    return -1;
> +
> +  if (verbose) {
> +    for (i = 0; lines[i] != NULL; ++i)
> +      fprintf (stderr, "ntfs_min_size: lines[%zu] =
\"%s\"\n", i, lines[i]);
> +  }
> +
> +  if (r == -1) {
> +    /* If volume is full, ntfsresize returns error. */
> +    for (i = 0; lines[i] != NULL; ++i) {
> +      if (strstr (lines[i], full_pattern))
Better use STRPREFIX here, which is what we use all around libguestfs.
> +        is_full = 1;
> +      else if ((p = strstr (lines[i], cluster_size_pattern))) {
> +        if (sscanf (p + strlen(cluster_size_pattern),
> +                    "%*[ ]:%" SCNd32, &cluster_size) != 1) {
> +          reply_with_error("Cannot parse cluster size");
> +          return -1;
> +        }
> +      }
> +      else if ((p = strstr (lines[i], volume_size_pattern))) {
> +        if (sscanf (p + strlen(volume_size_pattern),
> +                    "%" SCNd64, &volume_size) != 1) {
> +          reply_with_error("Cannot parse volume size");
> +          return -1;
> +        }
It sounds like these scans of the lines could be done using the
analyze_line helper in btrfs.c (which would need to be moved as common
code in guestfd.c). This way, the parsing of numbers could use
xstrtoul/xstrtoull (see btrfs.c:do_btrfs_subvolume_list) instead of
sscanf.
> +      }
> +    }
> +    if (is_full) {
> +      /* Ceil to cluster size */
> +      if (cluster_size == 0) {
> +        reply_with_error("Bad cluster size");
> +        return -1;
> +      }
> +      return (volume_size + cluster_size - 1) / cluster_size *
cluster_size;
Can you please explain (e.g. in a comment) what is this calculation for?
> +    }
> +
> +    reply_with_error ("%s", err);
> +    return -1;
> +  }
> +
> +  for (i = 0; lines[i] != NULL; ++i) {
> +    if ((p = strstr (lines[i], size_pattern))) {
> +      if (sscanf (p + strlen(size_pattern), "%" SCNd64,
&ret) != 1) {
> +        reply_with_error("Cannot parse minimum size");
> +        return -1;
Same notes as above wrt using analyze_line and xstrtoul.
> +      }
> +      return ret;
> +    }
> +  }
> +
> +  reply_with_error("Minimum size not found. Check output
format:\n%s", out);
> +  return -1;
> +}
> +
>  /* Takes optional arguments, consult optargs_bitmask. */
>  int
>  do_ntfsfix (const char *device, int clearbadsectors)
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 274ef3f..0646a16 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -12765,6 +12765,22 @@ Get the estimated minimum filesystem size of an
ext2/3/4 filesystem in blocks.
> 
>  See also L<resize2fs(8)>." };
> 
> +  { defaults with
> +    name = "vfs_min_size"; added = (1, 31, 18);
> +    style = RInt64 "sizeinbytes", [Mountable
"mountable"], [];
> +    proc_nr = Some 458;
> +    tests = [
> +      InitPartition, IfAvailable "ntfsprogs", TestRun(
> +        [["mkfs"; "ntfs"; "/dev/sda1";
""; "NOARG"; ""; ""; "NOARG"];
> +         ["vfs_min_size"; "/dev/sda1"]]), [];
> +    ];
> +    shortdesc = "get minimum filesystem size";
> +    longdesc = "\
> +Get the minimum size of filesystem in bytes.
> +This is the minimum possible size for filesystem shrinking.
> +
> +See also L<ntfsresize(8)>." };
> +
>  ]
> 
>  (* Non-API meta-commands available only in guestfish.
> diff --git a/po/POTFILES b/po/POTFILES
> index cd2c437..d90772a 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -49,6 +49,7 @@ daemon/file.c
>  daemon/fill.c
>  daemon/find.c
>  daemon/findfs.c
> +daemon/fs-min-size.c
>  daemon/fsck.c
>  daemon/fstrim.c
>  daemon/glob.c
> diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> index de2a00c..c92ddb6 100644
> --- a/src/MAX_PROC_NR
> +++ b/src/MAX_PROC_NR
> @@ -1 +1 @@
> -457
> +458
> --
Thanks,
-- 
Pino Toscano

On 10/19/2015 07:45 PM, Pino Toscano wrote:
> On Monday 19 October 2015 17:05:02 Maxim Perevedentsev wrote:
>> @@ -116,6 +116,7 @@ guestfsd_SOURCES = \
>>   	findfs.c \
>>   	fill.c \
>>   	find.c \
>> +	fs-min-size.c \
> Hm not really convinced by the file name chosen, but I cannot find
> better options. "size.c"? "fs-size.c"?
Neither can I. Current name, at least, gives a hint about what is 
inside. FS size is not so straightforward.

-- 
Your sincerely,
Maxim Perevedentsev

On 10/19/2015 07:45 PM, Pino Toscano wrote:
> On Monday 19 October 2015 17:05:02 Maxim Perevedentsev wrote:
>> +int64_t
>> +ntfs_min_size (const char *device)
>> +{
>> +  CLEANUP_FREE char *err = NULL, *out = NULL;
>> +  CLEANUP_FREE_STRING_LIST char **lines = NULL;
>> +  int r;
>> +  size_t i;
>> +  char *p;
>> +  int64_t ret, volume_size = 0;
> The scope of "ret" could be reduced to only the if below where it
is
> used.
Looking through code, I decided that declaring variables in the 
beginning of function is part of code style, so I followed it. Should I 
also move volume/cluster size declarations to branch where they are used?

-- 
Your sincerely,
Maxim Perevedentsev

On 10/19/2015 07:45 PM, Pino Toscano wrote:
> On Monday 19 October 2015 17:05:02 Maxim Perevedentsev wrote:
>
>> +  const char *size_pattern = "You might resize at ",
>> +             *full_pattern = "Volume is full",
>> +             *cluster_size_pattern = "Cluster size",
>> +             *volume_size_pattern = "Current volume size:";
>> +  int is_full = 0;
>> +  int32_t cluster_size = 0;
>> +
>> +  /* FS may be marked for check, so force ntfsresize */
>> +  r = command (&out, &err, str_ntfsresize, "--info",
"-ff", device, NULL);
>> +
>> +  lines = split_lines (out);
>> +  if (lines == NULL)
>> +    return -1;
>> +
>> +  if (verbose) {
>> +    for (i = 0; lines[i] != NULL; ++i)
>> +      fprintf (stderr, "ntfs_min_size: lines[%zu] =
\"%s\"\n", i, lines[i]);
>> +  }
>> +
>> +  if (r == -1) {
>> +    /* If volume is full, ntfsresize returns error. */
>> +    for (i = 0; lines[i] != NULL; ++i) {
>> +      if (strstr (lines[i], full_pattern))
> Better use STRPREFIX here, which is what we use all around libguestfs.
This is not at the beginning of line.
>
>> +        is_full = 1;
>> +      else if ((p = strstr (lines[i], cluster_size_pattern))) {
>> +        if (sscanf (p + strlen(cluster_size_pattern),
>> +                    "%*[ ]:%" SCNd32, &cluster_size) !=
1) {
>> +          reply_with_error("Cannot parse cluster size");
>> +          return -1;
>> +        }
>> +      }
>> +      else if ((p = strstr (lines[i], volume_size_pattern))) {
>> +        if (sscanf (p + strlen(volume_size_pattern),
>> +                    "%" SCNd64, &volume_size) != 1) {
>> +          reply_with_error("Cannot parse volume size");
>> +          return -1;
>> +        }
> It sounds like these scans of the lines could be done using the
> analyze_line helper in btrfs.c (which would need to be moved as common
> code in guestfd.c). This way, the parsing of numbers could use
> xstrtoul/xstrtoull (see btrfs.c:do_btrfs_subvolume_list) instead of
> sscanf.
This is the only place where it could be used for now. Does it worth 
moving the code? I guess this code won't be shorter or simpler when 
using analyze_line.
>> +    }
>> +
>> +    reply_with_error ("%s", err);
>> +    return -1;
>> +  }
>> +
>> +  for (i = 0; lines[i] != NULL; ++i) {
>> +    if ((p = strstr (lines[i], size_pattern))) {
>> +      if (sscanf (p + strlen(size_pattern), "%" SCNd64,
&ret) != 1) {
>> +        reply_with_error("Cannot parse minimum size");
>> +        return -1;
> Same notes as above wrt using analyze_line and xstrtoul.
No separator here. The string is "You might resize at 1231231 bytes"

-- 
Your sincerely,
Maxim Perevedentsev

On Tue, Oct 20, 2015 at 12:59:06PM +0300, Maxim Perevedentsev
wrote:
> 
> 
> On 10/19/2015 07:45 PM, Pino Toscano wrote:
> >On Monday 19 October 2015 17:05:02 Maxim Perevedentsev wrote:
> >>+int64_t
> >>+ntfs_min_size (const char *device)
> >>+{
> >>+  CLEANUP_FREE char *err = NULL, *out = NULL;
> >>+  CLEANUP_FREE_STRING_LIST char **lines = NULL;
> >>+  int r;
> >>+  size_t i;
> >>+  char *p;
> >>+  int64_t ret, volume_size = 0;
> >The scope of "ret" could be reduced to only the if below
where it is
> >used.
> Looking through code, I decided that declaring variables in the
> beginning of function is part of code style, so I followed it.
> Should I also move volume/cluster size declarations to branch where
> they are used?
The code style is to put variable decls at the top.

What Pino means here specifically is that the 'ret' could be
placed inside the later loop, ie:

+  for (i = 0; lines[i] != NULL; ++i) {
+    if ((p = strstr (lines[i], size_pattern))) {
       int64_t ret;

+      if (sscanf (p + strlen(size_pattern), "%" SCNd64, &ret) !=
1) {
+        reply_with_error("Cannot parse minimum size");
+        return -1;
+      }
+      return ret;
+    }

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Tue, Oct 20, 2015 at 01:28:23PM +0300, Maxim Perevedentsev
wrote:
> On 10/19/2015 07:45 PM, Pino Toscano wrote:
> >On Monday 19 October 2015 17:05:02 Maxim Perevedentsev wrote:
> >
> >>+  const char *size_pattern = "You might resize at ",
> >>+             *full_pattern = "Volume is full",
> >>+             *cluster_size_pattern = "Cluster size",
> >>+             *volume_size_pattern = "Current volume
size:";
> >>+  int is_full = 0;
> >>+  int32_t cluster_size = 0;
> >>+
> >>+  /* FS may be marked for check, so force ntfsresize */
> >>+  r = command (&out, &err, str_ntfsresize,
"--info", "-ff", device, NULL);
> >>+
> >>+  lines = split_lines (out);
> >>+  if (lines == NULL)
> >>+    return -1;
> >>+
> >>+  if (verbose) {
> >>+    for (i = 0; lines[i] != NULL; ++i)
> >>+      fprintf (stderr, "ntfs_min_size: lines[%zu] =
\"%s\"\n", i, lines[i]);
> >>+  }
> >>+
> >>+  if (r == -1) {
> >>+    /* If volume is full, ntfsresize returns error. */
> >>+    for (i = 0; lines[i] != NULL; ++i) {
> >>+      if (strstr (lines[i], full_pattern))
> >Better use STRPREFIX here, which is what we use all around libguestfs.
> This is not at the beginning of line.
> >
> >>+        is_full = 1;
> >>+      else if ((p = strstr (lines[i], cluster_size_pattern))) {
> >>+        if (sscanf (p + strlen(cluster_size_pattern),
> >>+                    "%*[ ]:%" SCNd32, &cluster_size)
!= 1) {
> >>+          reply_with_error("Cannot parse cluster size");
> >>+          return -1;
> >>+        }
> >>+      }
> >>+      else if ((p = strstr (lines[i], volume_size_pattern))) {
> >>+        if (sscanf (p + strlen(volume_size_pattern),
> >>+                    "%" SCNd64, &volume_size) != 1)
{
> >>+          reply_with_error("Cannot parse volume size");
> >>+          return -1;
> >>+        }
> >It sounds like these scans of the lines could be done using the
> >analyze_line helper in btrfs.c (which would need to be moved as common
> >code in guestfd.c). This way, the parsing of numbers could use
> >xstrtoul/xstrtoull (see btrfs.c:do_btrfs_subvolume_list) instead of
> >sscanf.
Although I think sscanf is fine, and certainly easier to use.

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/