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 <phrdina@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx",
> > - 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, &sb) < 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, &header)) < 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 0000000000..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 Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library 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
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library. If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#pragma once
> > +
> > +#include <sys/stat.h>
>
> #include <sys/types.h> for uid_t/gid_t instead, there's no use of the
> stat struct in the header, but that shouldn't be necessary at all if the
> format is probed fromt he buffer directly.
>
> > +
> > +#include "virstoragefile.h"
> > +
> > +/* Minimum header size required to probe all known formats with
> > + * virStorageFileProbeFormat, or obtain metadata from a known format.
> > + * Rounded to multiple of 512 (ISO has a 5-byte magic at offset
> > + * 32769). Some formats can be probed with fewer bytes. Although
> > + * some formats theoretically permit metadata that can rely on offsets
> > + * beyond this size, in practice that doesn't matter. */
> > +#define VIR_STORAGE_MAX_HEADER 0x8200
> > +
> > +int
> > +virStorageFileProbeGetMetadata(virStorageSourcePtr meta,
> > + char *buf,
> > + size_t len);
> > +
> > +int
> > +virStorageFileProbeFormat(const char *path,
> > + uid_t uid,
> > + gid_t gid);
>
> Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
>
> If you decide that it's too much hassle to shuffle this around, you can
> return the code opening the file back to the appropriate file later.
>
> (Or I'll do it, if you don't, but virstoragefileprobe.c will not
> 'open()' anything in the end)
Make sense, I'll look into it and post v3.
Thanks,
Pavel
Attachment:
signature.asc
Description: PGP signature
