Re: [libvirt PATCH v2 06/13] util: extract storage file probe code into virtstoragefileprobe.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux