Re: [libvirt PATCH v2 06/13] util: extract storage file probe code into virtstoragefileprobe.c
On Fri, Jan 22, 2021 at 10:09:27AM +0100, Pavel Hrdina wrote:
> On Fri, Jan 22, 2021 at 09:23:00AM +0100, Peter Krempa wrote:
> > On Thu, Jan 21, 2021 at 20:34:20 +0100, Pavel Hrdina wrote:
> > > This code is not directly relevant to virStorageSource so move it to
> > > separate file.
> > >
> > > Signed-off-by: Pavel Hrdina
> > > ---
> > > po/POTFILES.in| 1 +
> > > src/libvirt_private.syms | 6 +-
> > > src/qemu/qemu_driver.c| 1 +
> > > src/storage/storage_backend_gluster.c | 1 +
> > > src/storage/storage_util.c| 1 +
> > > src/util/meson.build | 1 +
> > > src/util/virstoragefile.c | 939 +
> > > src/util/virstoragefile.h | 11 -
> > > src/util/virstoragefileprobe.c| 967 ++
> > > src/util/virstoragefileprobe.h| 44 ++
> > > 10 files changed, 1026 insertions(+), 946 deletions(-)
> > > create mode 100644 src/util/virstoragefileprobe.c
> > > create mode 100644 src/util/virstoragefileprobe.h
> >
> > [...]
> >
> > > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> > > index 13a86f34e5..98a3222d09 100644
> > > --- a/src/util/virstoragefile.c
> > > +++ b/src/util/virstoragefile.c
> >
> > [...]
> >
> > > @@ -792,289 +146,6 @@ virStorageIsRelative(const char *backing)
> > > }
> > >
> > >
> > > -static int
> > > -virStorageFileProbeFormatFromBuf(const char *path,
> > > - char *buf,
> > > - size_t buflen)
> >
> > I'd prefer if this is the function exported from the new module ...
> >
> > > -{
> > > -int format = VIR_STORAGE_FILE_RAW;
> > > -size_t i;
> > > -int possibleFormat = VIR_STORAGE_FILE_RAW;
> > > -VIR_DEBUG("path=%s, buf=%p, buflen=%zu", path, buf, buflen);
> > > -
> > > -/* First check file magic */
> > > -for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) {
> > > -if (virStorageFileMatchesMagic(fileTypeInfo[i].magicOffset,
> > > - fileTypeInfo[i].magic,
> > > - buf, buflen)) {
> > > -if
> > > (!virStorageFileMatchesVersion(fileTypeInfo[i].versionOffset,
> > > -
> > > fileTypeInfo[i].versionSize,
> > > -
> > > fileTypeInfo[i].versionNumbers,
> > > - fileTypeInfo[i].endian,
> > > - buf, buflen)) {
> > > -possibleFormat = i;
> > > -continue;
> > > -}
> > > -format = i;
> > > -goto cleanup;
> > > -}
> > > -}
> > > -
> > > -if (possibleFormat != VIR_STORAGE_FILE_RAW)
> > > -VIR_WARN("File %s matches %s magic, but version is wrong. "
> > > - "Please report new version to libvir-list@redhat.com",
> > > - path, virStorageFileFormatTypeToString(possibleFormat));
> > > -
> > > - cleanup:
> > > -VIR_DEBUG("format=%d", format);
> > > -return format;
> > > -}
> >
> > [...]
> >
> > > -/**
> > > - * virStorageFileProbeFormat:
> > > - *
> > > - * Probe for the format of 'path', returning the detected
> > > - * disk format.
> > > - *
> > > - * Callers are advised never to trust the returned 'format'
> > > - * unless it is listed as VIR_STORAGE_FILE_RAW, since a
> > > - * malicious guest can turn a raw file into any other non-raw
> > > - * format at will.
> > > - *
> > > - * Best option: Don't use this function
> > > - */
> > > -int
> > > -virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid)
> > > -{
> >
> > ... and not this.
> >
> > > -struct stat sb;
> > > -ssize_t len = VIR_STORAGE_MAX_HEADER;
> > > -VIR_AUTOCLOSE fd = -1;
> > > -g_autofree char *header = NULL;
> > > -
> > > -if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
> >
> > Specifically the new module should not ever touch any real storage and
> > should just be a self-contained prober of metadata froma buffer.
> >
> > > -virReportSystemError(-fd, _("Failed to open file '%s'"), path);
> > > -return -1;
> > > -}
> > > -
> > > -if (fstat(fd, ) < 0) {
> > > -virReportSystemError(errno, _("cannot stat file '%s'"), path);
> > > -return -1;
> > > -}
> > > -
> > > -/* No header to probe for directories */
> > > -if (S_ISDIR(sb.st_mode))
> > > -return VIR_STORAGE_FILE_DIR;
> > > -
> > > -if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
> > > -virReportSystemError(errno, _("cannot set to start of '%s'"),
> > > path);
> > > -return -1;
> > > -}
> > > -
> > > -if ((len = virFileReadHeaderFD(fd, len, )) < 0) {
> > > -virReportSystemError(errno, _("cannot read header '%s'"), path);
> > > -return -1;
> > > -}
> > > -
>
Re: [libvirt PATCH v2 06/13] util: extract storage file probe code into virtstoragefileprobe.c
On Fri, Jan 22, 2021 at 09:23:00AM +0100, Peter Krempa wrote:
> On Thu, Jan 21, 2021 at 20:34:20 +0100, Pavel Hrdina wrote:
> > This code is not directly relevant to virStorageSource so move it to
> > separate file.
> >
> > Signed-off-by: Pavel Hrdina
> > ---
> > po/POTFILES.in| 1 +
> > src/libvirt_private.syms | 6 +-
> > src/qemu/qemu_driver.c| 1 +
> > src/storage/storage_backend_gluster.c | 1 +
> > src/storage/storage_util.c| 1 +
> > src/util/meson.build | 1 +
> > src/util/virstoragefile.c | 939 +
> > src/util/virstoragefile.h | 11 -
> > src/util/virstoragefileprobe.c| 967 ++
> > src/util/virstoragefileprobe.h| 44 ++
> > 10 files changed, 1026 insertions(+), 946 deletions(-)
> > create mode 100644 src/util/virstoragefileprobe.c
> > create mode 100644 src/util/virstoragefileprobe.h
>
> [...]
>
> > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> > index 13a86f34e5..98a3222d09 100644
> > --- a/src/util/virstoragefile.c
> > +++ b/src/util/virstoragefile.c
>
> [...]
>
> > @@ -792,289 +146,6 @@ virStorageIsRelative(const char *backing)
> > }
> >
> >
> > -static int
> > -virStorageFileProbeFormatFromBuf(const char *path,
> > - char *buf,
> > - size_t buflen)
>
> I'd prefer if this is the function exported from the new module ...
>
> > -{
> > -int format = VIR_STORAGE_FILE_RAW;
> > -size_t i;
> > -int possibleFormat = VIR_STORAGE_FILE_RAW;
> > -VIR_DEBUG("path=%s, buf=%p, buflen=%zu", path, buf, buflen);
> > -
> > -/* First check file magic */
> > -for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) {
> > -if (virStorageFileMatchesMagic(fileTypeInfo[i].magicOffset,
> > - fileTypeInfo[i].magic,
> > - buf, buflen)) {
> > -if
> > (!virStorageFileMatchesVersion(fileTypeInfo[i].versionOffset,
> > - fileTypeInfo[i].versionSize,
> > -
> > fileTypeInfo[i].versionNumbers,
> > - fileTypeInfo[i].endian,
> > - buf, buflen)) {
> > -possibleFormat = i;
> > -continue;
> > -}
> > -format = i;
> > -goto cleanup;
> > -}
> > -}
> > -
> > -if (possibleFormat != VIR_STORAGE_FILE_RAW)
> > -VIR_WARN("File %s matches %s magic, but version is wrong. "
> > - "Please report new version to libvir-list@redhat.com",
> > - path, virStorageFileFormatTypeToString(possibleFormat));
> > -
> > - cleanup:
> > -VIR_DEBUG("format=%d", format);
> > -return format;
> > -}
>
> [...]
>
> > -/**
> > - * virStorageFileProbeFormat:
> > - *
> > - * Probe for the format of 'path', returning the detected
> > - * disk format.
> > - *
> > - * Callers are advised never to trust the returned 'format'
> > - * unless it is listed as VIR_STORAGE_FILE_RAW, since a
> > - * malicious guest can turn a raw file into any other non-raw
> > - * format at will.
> > - *
> > - * Best option: Don't use this function
> > - */
> > -int
> > -virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid)
> > -{
>
> ... and not this.
>
> > -struct stat sb;
> > -ssize_t len = VIR_STORAGE_MAX_HEADER;
> > -VIR_AUTOCLOSE fd = -1;
> > -g_autofree char *header = NULL;
> > -
> > -if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
>
> Specifically the new module should not ever touch any real storage and
> should just be a self-contained prober of metadata froma buffer.
>
> > -virReportSystemError(-fd, _("Failed to open file '%s'"), path);
> > -return -1;
> > -}
> > -
> > -if (fstat(fd, ) < 0) {
> > -virReportSystemError(errno, _("cannot stat file '%s'"), path);
> > -return -1;
> > -}
> > -
> > -/* No header to probe for directories */
> > -if (S_ISDIR(sb.st_mode))
> > -return VIR_STORAGE_FILE_DIR;
> > -
> > -if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
> > -virReportSystemError(errno, _("cannot set to start of '%s'"),
> > path);
> > -return -1;
> > -}
> > -
> > -if ((len = virFileReadHeaderFD(fd, len, )) < 0) {
> > -virReportSystemError(errno, _("cannot read header '%s'"), path);
> > -return -1;
> > -}
> > -
> > -return virStorageFileProbeFormatFromBuf(path, header, len);
> > -}
>
> [...]
>
> > diff --git a/src/util/virstoragefileprobe.h b/src/util/virstoragefileprobe.h
> > new file mode 100644
> > index 00..2b94a4ae51
> > --- /dev/null
> > +++ b/src/util/virstoragefileprobe.h
> > @@ -0,0 +1,44 @@
> > +/*
> > + *
Re: [libvirt PATCH v2 06/13] util: extract storage file probe code into virtstoragefileprobe.c
On Thu, Jan 21, 2021 at 20:34:20 +0100, Pavel Hrdina wrote:
> This code is not directly relevant to virStorageSource so move it to
> separate file.
>
> Signed-off-by: Pavel Hrdina
> ---
> po/POTFILES.in| 1 +
> src/libvirt_private.syms | 6 +-
> src/qemu/qemu_driver.c| 1 +
> src/storage/storage_backend_gluster.c | 1 +
> src/storage/storage_util.c| 1 +
> src/util/meson.build | 1 +
> src/util/virstoragefile.c | 939 +
> src/util/virstoragefile.h | 11 -
> src/util/virstoragefileprobe.c| 967 ++
> src/util/virstoragefileprobe.h| 44 ++
> 10 files changed, 1026 insertions(+), 946 deletions(-)
> create mode 100644 src/util/virstoragefileprobe.c
> create mode 100644 src/util/virstoragefileprobe.h
[...]
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 13a86f34e5..98a3222d09 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
[...]
> @@ -792,289 +146,6 @@ virStorageIsRelative(const char *backing)
> }
>
>
> -static int
> -virStorageFileProbeFormatFromBuf(const char *path,
> - char *buf,
> - size_t buflen)
I'd prefer if this is the function exported from the new module ...
> -{
> -int format = VIR_STORAGE_FILE_RAW;
> -size_t i;
> -int possibleFormat = VIR_STORAGE_FILE_RAW;
> -VIR_DEBUG("path=%s, buf=%p, buflen=%zu", path, buf, buflen);
> -
> -/* First check file magic */
> -for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) {
> -if (virStorageFileMatchesMagic(fileTypeInfo[i].magicOffset,
> - fileTypeInfo[i].magic,
> - buf, buflen)) {
> -if (!virStorageFileMatchesVersion(fileTypeInfo[i].versionOffset,
> - fileTypeInfo[i].versionSize,
> - fileTypeInfo[i].versionNumbers,
> - fileTypeInfo[i].endian,
> - buf, buflen)) {
> -possibleFormat = i;
> -continue;
> -}
> -format = i;
> -goto cleanup;
> -}
> -}
> -
> -if (possibleFormat != VIR_STORAGE_FILE_RAW)
> -VIR_WARN("File %s matches %s magic, but version is wrong. "
> - "Please report new version to libvir-list@redhat.com",
> - path, virStorageFileFormatTypeToString(possibleFormat));
> -
> - cleanup:
> -VIR_DEBUG("format=%d", format);
> -return format;
> -}
[...]
> -/**
> - * virStorageFileProbeFormat:
> - *
> - * Probe for the format of 'path', returning the detected
> - * disk format.
> - *
> - * Callers are advised never to trust the returned 'format'
> - * unless it is listed as VIR_STORAGE_FILE_RAW, since a
> - * malicious guest can turn a raw file into any other non-raw
> - * format at will.
> - *
> - * Best option: Don't use this function
> - */
> -int
> -virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid)
> -{
... and not this.
> -struct stat sb;
> -ssize_t len = VIR_STORAGE_MAX_HEADER;
> -VIR_AUTOCLOSE fd = -1;
> -g_autofree char *header = NULL;
> -
> -if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
Specifically the new module should not ever touch any real storage and
should just be a self-contained prober of metadata froma buffer.
> -virReportSystemError(-fd, _("Failed to open file '%s'"), path);
> -return -1;
> -}
> -
> -if (fstat(fd, ) < 0) {
> -virReportSystemError(errno, _("cannot stat file '%s'"), path);
> -return -1;
> -}
> -
> -/* No header to probe for directories */
> -if (S_ISDIR(sb.st_mode))
> -return VIR_STORAGE_FILE_DIR;
> -
> -if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
> -virReportSystemError(errno, _("cannot set to start of '%s'"), path);
> -return -1;
> -}
> -
> -if ((len = virFileReadHeaderFD(fd, len, )) < 0) {
> -virReportSystemError(errno, _("cannot read header '%s'"), path);
> -return -1;
> -}
> -
> -return virStorageFileProbeFormatFromBuf(path, header, len);
> -}
[...]
> diff --git a/src/util/virstoragefileprobe.h b/src/util/virstoragefileprobe.h
> new file mode 100644
> index 00..2b94a4ae51
> --- /dev/null
> +++ b/src/util/virstoragefileprobe.h
> @@ -0,0 +1,44 @@
> +/*
> + * virstoragefileprobe.h: file utility functions for FS storage backend
> + *
> + * Copyright (C) 2007-2009, 2012-2016 Red Hat, Inc.
> + * Copyright (C) 2007-2008 Daniel P. Berrange
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software
