[libvirt PATCH v2 06/13] util: extract storage file probe code into virtstoragefileprobe.c
Pavel Hrdina
phrdina at redhat.com
Fri Jan 22 10:07:58 UTC 2021
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 <phrdina at redhat.com>
> > > ---
> > > 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 at 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, &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 at redhat.com>
> >
> > 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.
So I looked into it and the best what we can do is remove
virStorageFileProbeFormat completely. I've discussed it with Peter
and it will be done as followup so I'll take your RB for now.
Thanks for the review, I'll push this series shortly.
Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210122/5e4cd5c3/attachment-0001.sig>
More information about the libvir-list
mailing list