Re: [PATCH] hw/ufs: Fix incorrect register fields
On Tue, Oct 10, 2023 at 1:11 PM Jeuk Kim wrote:
>
> From: Jeuk Kim
>
> This patch fixes invalid ufs register fields.
> This fixes an issue reported by Bin Meng that
> caused ufs to fail over riscv.
>
Fixes: bc4e68d362ec ("hw/ufs: Initial commit for emulated
Universal-Flash-Storage")
Reported-by: Bin Meng
> Signed-off-by: Jeuk Kim
> ---
> include/block/ufs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Reviewed-by: Bin Meng
Tested-by: Bin Meng
Re: [PATCH 10/20] hw/arm: Open-code pflash_cfi01_register()
On Thu, Jan 5, 2023 at 6:43 AM Philippe Mathieu-Daudé wrote: > > pflash_cfi01_register() hides an implicit sysbus mapping of > MMIO region #0. This is not practical in a heterogeneous world > where multiple cores use different address spaces. In order to > remove to remove pflash_cfi01_register() from the pflash API, duplicated "to remove" > open-code it as a qdev creation call followed by an explicit > sysbus mapping. > > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/arm/collie.c | 15 +-- > hw/arm/gumstix.c | 19 +-- > hw/arm/mainstone.c | 13 - > hw/arm/omap_sx1.c| 22 ++ > hw/arm/versatilepb.c | 13 - > hw/arm/z2.c | 10 +++--- > 6 files changed, 59 insertions(+), 33 deletions(-) > Otherwise, Reviewed-by: Bin Meng
Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
Hi Thomas,
On Wed, Sep 21, 2022 at 8:24 PM Thomas Huth wrote:
>
> On 21/09/2022 14.18, Bin Meng wrote:
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 9:28 PM Bin Meng wrote:
> >>
> >> At present packaging the required DLLs of QEMU executables is a
> >> manual process, and error prone.
> >>
> >> Improve scripts/nsis.py by adding a logic to automatically package
> >> required DLLs of QEMU executables.
> >>
> >> 'make installer' is tested in the cross-build on Linux in CI, but
> >> not in the Windows native build. Update CI to test the installer
> >> generation on Windows too.
> >>
> >> During testing a 32-bit build issue was exposed in block/nfs.c and
> >> the fix is included in this series.
> >>
> >>
> >> Bin Meng (7):
> >>scripts/nsis.py: Drop the unnecessary path separator
> >>scripts/nsis.py: Fix destination directory name when invoked on
> >> Windows
> >>scripts/nsis.py: Automatically package required DLLs of QEMU
> >> executables
> >>.gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
> >>block/nfs: Fix 32-bit Windows build
> >>.gitlab-ci.d/windows.yml: Unify the prerequisite packages
> >>.gitlab-ci.d/windows.yml: Test 'make installer' in the CI
> >>
> >> meson.build | 1 +
> >> block/nfs.c | 8 ++
> >> .gitlab-ci.d/windows.yml | 40 ---
> >> scripts/nsis.py | 60 +---
> >> 4 files changed, 89 insertions(+), 20 deletions(-)
> >>
> >
> > I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
> > sed processing in the 64-bit build")
> >
> > What about other patches?
>
> I hope that Stefan Weil (our W32 maintainer) could have a look at these
> first...
>
Stefan has reviewed / tested patch 1-3. Not sure who is going to queue
these 3 patches?
Regards,
Bin
Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
On Thu, Oct 27, 2022 at 3:55 PM Kevin Wolf wrote: > > Am 27.10.2022 um 04:45 hat Bin Meng geschrieben: > > Hi Kevin, > > [...] > > Will you queue this patch via the block tree? > > Just to be sure, you mean only patch 5? Yes, I can do that. > Yes, only this one. Thank you. Regards, Bin
Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
Hi Kevin,
On Sat, Sep 24, 2022 at 9:19 AM Bin Meng wrote:
>
> Hi,
>
> On Wed, Sep 21, 2022 at 8:10 PM Meng, Bin wrote:
> >
> > -Original Message-
> > From: Philippe Mathieu-Daudé On Behalf
> > Of Philippe Mathieu-Daudé
> > Sent: Sunday, September 18, 2022 5:32 AM
> > To: Bin Meng ; qemu-de...@nongnu.org
> > Cc: Meng, Bin ; Hanna Reitz ;
> > Kevin Wolf ; Peter Lieven ;
> > qemu-block@nongnu.org
> > Subject: Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
> >
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> >
> > On 8/9/22 15:28, Bin Meng wrote:
> > > From: Bin Meng
> > >
> > > libnfs.h declares nfs_fstat() as the following for win32:
> > >
> > >int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
> > > struct __stat64 *st);
> > >
> > > The 'st' parameter should be of type 'struct __stat64'. The codes
> > > happen to build successfully for 64-bit Windows, but it does not build
> > > for 32-bit Windows.
> > >
> > > Fixes: 6542aa9c75bc ("block: add native support for NFS")
> > > Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for
> > > read-only files")
> > > Signed-off-by: Bin Meng
> > > ---
> > >
> > > block/nfs.c | 8
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/block/nfs.c b/block/nfs.c index 444c40b458..d5d67937dd
> > > 100644
> > > --- a/block/nfs.c
> > > +++ b/block/nfs.c
> > > @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client,
> > > BlockdevOptionsNfs *opts,
> > > int flags, int open_flags, Error **errp)
> > > {
> > > int64_t ret = -EINVAL;
> > > +#ifdef _WIN32
> > > +struct __stat64 st;
> > > +#else
> > > struct stat st;
> > > +#endif
> > > char *file = NULL, *strp = NULL;
> > >
> > > qemu_mutex_init(>mutex); @@ -781,7 +785,11 @@ static int
> > > nfs_reopen_prepare(BDRVReopenState *state,
> > > BlockReopenQueue *queue, Error **errp)
> > > {
> > > NFSClient *client = state->bs->opaque;
> > > +#ifdef _WIN32
> > > +struct __stat64 st;
> > > +#else
> > > struct stat st;
> > > +#endif
> > > int ret = 0;
> > >
> > > if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs))
> > > {
> >
> > What about the field in struct NFSRPC?
> >
> > NFSRPC::struct stat is used in nfs_get_allocated_file_size_cb() and
> > nfs_get_allocated_file_size() that are not built on win32, so there is no
> > problem.
> >
>
> Any further comments?
>
Will you queue this patch via the block tree?
Regards,
Bin
[PATCH v4 3/3] util/aio-win32: Correct the event array size in aio_poll()
From: Bin Meng
WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
object handles. Correct the event array size in aio_poll() and
add a assert() to ensure it does not cause out of bound access.
Signed-off-by: Bin Meng
Reviewed-by: Stefan Weil
Reviewed-by: Marc-André Lureau
Reviewed-by: Daniel P. Berrangé
---
(no changes since v2)
Changes in v2:
- change 'count' to unsigned
util/aio-win32.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 44003d645e..80cfe012ad 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -326,9 +326,9 @@ void aio_dispatch(AioContext *ctx)
bool aio_poll(AioContext *ctx, bool blocking)
{
AioHandler *node;
-HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
+HANDLE events[MAXIMUM_WAIT_OBJECTS];
bool progress, have_select_revents, first;
-int count;
+unsigned count;
int timeout;
/*
@@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
QLIST_FOREACH_RCU(node, >aio_handlers, node) {
if (!node->deleted && node->io_notify
&& aio_node_check(ctx, node->is_external)) {
+assert(count < MAXIMUM_WAIT_OBJECTS);
events[count++] = event_notifier_get_handle(node->e);
}
}
--
2.34.1
Re: [PATCH v5 00/18] tests/qtest: Enable running qtest on Windows
On Wed, Oct 19, 2022 at 12:00 AM Alex Bennée wrote: > > > Bin Meng writes: > > > Hi Alex, > > > > On Fri, Oct 7, 2022 at 1:31 PM Bin Meng wrote: > >> > >> On Fri, Oct 7, 2022 at 4:35 AM Alex Bennée wrote: > >> > > >> > > >> > Bin Meng writes: > >> > > >> > > In preparation to adding virtio-9p support on Windows, this series > >> > > enables running qtest on Windows, so that we can run the virtio-9p > >> > > tests on Windows to make sure it does not break accidently. > >> > > >> > I'm happy to take this whole series through my testing/next however I > >> > don't have working CI for the month so need to wait for my minutes to > >> > reset. Have you done a full CI run* with this? > >> > > >> > >> Yes, gitlab CI passed. > >> > >> > (*make sure any CI run is only on a repo forked from > >> > https://gitlab.com/qemu-project as you won't get the discount cost > >> > factor otherwise) > >> > > > > > Patch 4 and 10 are already applied in the mainline by Thomas. > > > > Daniel will queue patch 14, 15, 16. > > > > Could you please help queue patch 1, 2, 3, 5, 6, 7, 9, 13 from this > > series? > > I've queued those except 13 into my testing/next. There was a merge > failure so I guess a dependency on another patch? > Thank you Alex. Indeed patch 13 depends on a previous patch. Please ignore. I will rebase in the next version. Regards, Bin
Re: [PATCH v3 7/9] hw/ppc/e500: Implement pflash handling
On Tue, Oct 18, 2022 at 3:46 PM Bernhard Beschow wrote:
>
> Am 16. Oktober 2022 14:15:09 UTC schrieb BALATON Zoltan :
> >On Sun, 16 Oct 2022, Bernhard Beschow wrote:
> >> Allows e500 boards to have their root file system reside on flash using
> >> only builtin devices located in the eLBC memory region.
> >>
> >> Note that the flash memory area is only created when a -pflash argument is
> >> given, and that the size is determined by the given file. The idea is to
> >> put users into control.
> >>
> >> Signed-off-by: Bernhard Beschow
> >> ---
> >> docs/system/ppc/ppce500.rst | 16 ++
> >> hw/ppc/Kconfig | 1 +
> >> hw/ppc/e500.c | 62 +
> >> 3 files changed, 79 insertions(+)
> >>
> >> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> >> index ba6bcb7314..99d2c680d6 100644
> >> --- a/docs/system/ppc/ppce500.rst
> >> +++ b/docs/system/ppc/ppce500.rst
> >> @@ -165,3 +165,19 @@ if “-device eTSEC” is given to QEMU:
> >> .. code-block:: bash
> >>
> >> -netdev tap,ifname=tap0,script=no,downscript=no,id=net0 -device
> >> eTSEC,netdev=net0
> >> +
> >> +Root file system on flash drive
> >> +---
> >> +
> >> +Rather than using a root file system on ram disk, it is possible to have
> >> it on
> >> +CFI flash. Given an ext2 image whose size must be a power of two, it can
> >> be used
> >> +as follows:
> >> +
> >> +.. code-block:: bash
> >> +
> >> + $ qemu-system-ppc{64|32} -M ppce500 -cpu e500mc -smp 4 -m 2G \
> >
> >We have qemu-system-ppc and qemu-system-ppc64 not qemu-system-ppc32 so maybe
> >qemu-system-ppc[64] even though that looks odd so maybe just qemu-system-ppc
> >and then people should know that ppc64 includes ppc config as well.
>
> True. This naming is used elsewhere in this document, so we kept it
> consistent. I think that users will get it either way.
>
> @Bin: Any thoughts?
>
How about a separate patch to remove the {64 | 32} suffix, and just
use qemu-system-ppc64 consistently since the *ppc64 executable can run
32-bit ppc codes too?
Regards,
Bin
Re: [PATCH v5 00/18] tests/qtest: Enable running qtest on Windows
Hi Alex, On Fri, Oct 7, 2022 at 1:31 PM Bin Meng wrote: > > On Fri, Oct 7, 2022 at 4:35 AM Alex Bennée wrote: > > > > > > Bin Meng writes: > > > > > In preparation to adding virtio-9p support on Windows, this series > > > enables running qtest on Windows, so that we can run the virtio-9p > > > tests on Windows to make sure it does not break accidently. > > > > I'm happy to take this whole series through my testing/next however I > > don't have working CI for the month so need to wait for my minutes to > > reset. Have you done a full CI run* with this? > > > > Yes, gitlab CI passed. > > > (*make sure any CI run is only on a repo forked from > > https://gitlab.com/qemu-project as you won't get the discount cost > > factor otherwise) > > Patch 4 and 10 are already applied in the mainline by Thomas. Daniel will queue patch 14, 15, 16. Could you please help queue patch 1, 2, 3, 5, 6, 7, 9, 13 from this series? I will rework the rest of the patches. Regards, Bin
Re: [PATCH v6 2/2] block: Refactor get_tmp_filename()
On Mon, Oct 10, 2022 at 12:05 PM Bin Meng wrote: > > At present there are two callers of get_tmp_filename() and they are > inconsistent. > > One does: > > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ > char *tmp_filename = g_malloc0(PATH_MAX + 1); > ... > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1); > > while the other does: > > s->qcow_filename = g_malloc(PATH_MAX); > ret = get_tmp_filename(s->qcow_filename, PATH_MAX); > > As we can see different 'size' arguments are passed. There are also > platform specific implementations inside the function, and the use > of snprintf is really undesirable. > > The function name is also misleading. It creates a temporary file, > not just a filename. > > Refactor this routine by changing its name and signature to: > > char *create_tmp_file(Error **errp) > > and use g_get_tmp_dir() / g_mkstemp() for a consistent implementation. > > While we are here, add some comments to mention that /var/tmp is > preferred over /tmp on non-win32 hosts. > > Signed-off-by: Bin Meng > --- > > Changes in v6: > - use g_mkstemp() and stick to use /var/tmp for non-win32 hosts > > Changes in v5: > - minor change in the commit message > - add some notes in the function comment block > - add g_autofree for tmp_filename > > Changes in v4: > - Rename the function to create_tmp_file() and take "Error **errp" as > a parameter, so that callers can pass errp all the way down to this > routine. > - Commit message updated to reflect the latest change > > Changes in v3: > - Do not use errno directly, instead still let get_tmp_filename() return > a negative number to indicate error > > Changes in v2: > - Use g_autofree and g_steal_pointer > > include/block/block_int-common.h | 2 +- > block.c | 56 +--- > block/vvfat.c| 7 ++-- > 3 files changed, 34 insertions(+), 31 deletions(-) > Any comments?
Re: [PATCH] tests/unit/test-image-locking: Fix handling of temporary files
On Wed, Oct 12, 2022 at 4:59 PM Thomas Huth wrote:
>
> test-image-locking leaves some temporary files around - clean
> them up. While we're at it, test-image-locking is a unit test,
> so it should not use "qtest.*" for temporary file names. Give
> them better names instead, so that it clear where the temporary
> files come from.
>
> Signed-off-by: Thomas Huth
> ---
> tests/unit/test-image-locking.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
Fixes: aef96d7d4f0b ("tests: Add unit tests for image locking")
Reviewed-by: Bin Meng
[PATCH v6 1/2] block: Ignore close() failure in get_tmp_filename()
The temporary file has been created and is ready for use. Checking
return value of close() does not seem useful. The file descriptor
is almost certainly closed; see close(2) under "Dealing with error
returns from close()".
Let's simply ignore close() failure here.
Suggested-by: Markus Armbruster
Signed-off-by: Bin Meng
Reviewed-by: Markus Armbruster
---
(no changes since v5)
Changes in v5:
- new patch: "block: Ignore close() failure in get_tmp_filename()"
block.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/block.c b/block.c
index bc85f46eed..582c205307 100644
--- a/block.c
+++ b/block.c
@@ -886,10 +886,7 @@ int get_tmp_filename(char *filename, int size)
if (fd < 0) {
return -errno;
}
-if (close(fd) != 0) {
-unlink(filename);
-return -errno;
-}
+close(fd);
return 0;
#endif
}
--
2.25.1
[PATCH v6 2/2] block: Refactor get_tmp_filename()
At present there are two callers of get_tmp_filename() and they are
inconsistent.
One does:
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char *tmp_filename = g_malloc0(PATH_MAX + 1);
...
ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
while the other does:
s->qcow_filename = g_malloc(PATH_MAX);
ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
As we can see different 'size' arguments are passed. There are also
platform specific implementations inside the function, and the use
of snprintf is really undesirable.
The function name is also misleading. It creates a temporary file,
not just a filename.
Refactor this routine by changing its name and signature to:
char *create_tmp_file(Error **errp)
and use g_get_tmp_dir() / g_mkstemp() for a consistent implementation.
While we are here, add some comments to mention that /var/tmp is
preferred over /tmp on non-win32 hosts.
Signed-off-by: Bin Meng
---
Changes in v6:
- use g_mkstemp() and stick to use /var/tmp for non-win32 hosts
Changes in v5:
- minor change in the commit message
- add some notes in the function comment block
- add g_autofree for tmp_filename
Changes in v4:
- Rename the function to create_tmp_file() and take "Error **errp" as
a parameter, so that callers can pass errp all the way down to this
routine.
- Commit message updated to reflect the latest change
Changes in v3:
- Do not use errno directly, instead still let get_tmp_filename() return
a negative number to indicate error
Changes in v2:
- Use g_autofree and g_steal_pointer
include/block/block_int-common.h | 2 +-
block.c | 56 +---
block/vvfat.c| 7 ++--
3 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..d7c0a7e96f 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild *child)
}
int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
-int get_tmp_filename(char *filename, int size);
+char *create_tmp_file(Error **errp);
void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
QDict *options);
diff --git a/block.c b/block.c
index 582c205307..8eeaa5623e 100644
--- a/block.c
+++ b/block.c
@@ -860,35 +860,42 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
/*
* Create a uniquely-named empty temporary file.
- * Return 0 upon success, otherwise a negative errno value.
+ * Return the actual file name used upon success, otherwise NULL.
+ * This string should be freed with g_free() when not needed any longer.
+ *
+ * Note: creating a temporary file for the caller to (re)open is
+ * inherently racy. Use g_file_open_tmp() instead whenever practical.
*/
-int get_tmp_filename(char *filename, int size)
+char *create_tmp_file(Error **errp)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
int fd;
const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
+g_autofree char *filename = NULL;
+
+tmpdir = g_get_tmp_dir();
+#ifndef _WIN32
+/*
+ * See commit 69bef79 ("block: use /var/tmp instead of /tmp for -snapshot")
+ *
+ * This function is used to create temporary disk images (like -snapshot),
+ * so the files can become very large. /tmp is often a tmpfs where as
+ * /var/tmp is usually on a disk, so more appropriate for disk images.
+ */
+if (!g_strcmp0(tmpdir, "/tmp")) {
tmpdir = "/var/tmp";
}
-if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
-return -EOVERFLOW;
-}
-fd = mkstemp(filename);
+#endif
+
+filename = g_strdup_printf("%s/vl.XX", tmpdir);
+fd = g_mkstemp(filename);
if (fd < 0) {
-return -errno;
+error_setg_errno(errp, -errno, "Could not open temporary file '%s'",
+ filename);
+return NULL;
}
close(fd);
-return 0;
-#endif
+
+return g_steal_pointer();
}
/*
@@ -3714,8 +3721,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
QDict *snapshot_options,
Error **errp)
{
-/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
-char *tmp_filename = g_malloc0(PATH_MAX + 1);
+g_autofree char *tm
Re: [PATCH v2 09/13] hw/ppc/e500: Implement pflash handling
_create_devtree, );
>
> +sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
> +));
> +if (sbdev) {
> +assert(!ambiguous);
> +create_devtree_flash(sbdev, );
> +}
> +
> g_free(node);
> }
>
> @@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
> unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
> IrqLines *irqs;
> DeviceState *dev, *mpicdev;
> +DriveInfo *dinfo;
> CPUPPCState *firstenv = NULL;
> MemoryRegion *ccsr_addr_space;
> SysBusDevice *s;
> @@ -1024,6 +1061,45 @@ void ppce500_init(MachineState *machine)
> pmc->platform_bus_base,
> >pbus_dev->mmio);
>
> +dinfo = drive_get(IF_PFLASH, 0, 0);
> +if (dinfo) {
> +BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> +BlockDriverState *bs = blk_bs(blk);
> +uint64_t size = bdrv_getlength(bs);
> +uint64_t mmio_size = pms->pbus_dev->mmio.size;
> +uint32_t sector_len = 64 * KiB;
> +
> +if (ctpop64(size) != 1) {
> +error_report("Size of pflash file must be a power of two.");
> +exit(1);
> +}
> +
> +if (size > mmio_size) {
> +error_report("Size of pflash file must not be bigger than %"
> PRIu64
> + " bytes.", mmio_size);
> +exit(1);
> +}
> +
> +assert(QEMU_IS_ALIGNED(size, sector_len));
> +
> +dev = qdev_new(TYPE_PFLASH_CFI01);
> +qdev_prop_set_drive(dev, "drive", blk);
> +qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
> +qdev_prop_set_uint64(dev, "sector-length", sector_len);
> +qdev_prop_set_uint8(dev, "width", 2);
> +qdev_prop_set_bit(dev, "big-endian", true);
> +qdev_prop_set_uint16(dev, "id0", 0x89);
> +qdev_prop_set_uint16(dev, "id1", 0x18);
> +qdev_prop_set_uint16(dev, "id2", 0x);
> +qdev_prop_set_uint16(dev, "id3", 0x0);
> +qdev_prop_set_string(dev, "name", "e500.flash");
> +s = SYS_BUS_DEVICE(dev);
> +sysbus_realize_and_unref(s, _fatal);
> +
> +memory_region_add_subregion(>pbus_dev->mmio, 0,
> +sysbus_mmio_get_region(s, 0));
> +}
> +
> /*
> * Smart firmware defaults ahead!
> *
Otherwise LGTM:
Reviewed-by: Bin Meng
Re: [PATCH v2 00/13] ppc/e500: Add support for two types of flash, cleanup
On Sun, Oct 9, 2022 at 12:11 AM Bernhard Beschow wrote: > > Am 4. Oktober 2022 12:43:35 UTC schrieb Daniel Henrique Barboza > : > >Hey, > > > >On 10/3/22 18:27, Philippe Mathieu-Daudé wrote: > >> Hi Daniel, > >> > >> On 3/10/22 22:31, Bernhard Beschow wrote: > >>> Cover letter: > >>> ~ > >>> > >>> This series adds support for -pflash and direct SD card access to the > >>> PPC e500 boards. The idea is to increase compatibility with "real" > >>> firmware > >>> images where only the bare minimum of drivers is compiled in. > >> > >>> Bernhard Beschow (13): > >>>hw/ppc/meson: Allow e500 boards to be enabled separately > >>>hw/gpio/meson: Introduce dedicated config switch for hw/gpio/mpc8xxx > >>>docs/system/ppc/ppce500: Add heading for networking chapter > >>>hw/ppc/e500: Reduce usage of sysbus API > >>>hw/ppc/mpc8544ds: Rename wrongly named method > >>>hw/ppc/mpc8544ds: Add platform bus > >>>hw/ppc/e500: Remove if statement which is now always true > >> > >> This first part is mostly reviewed and can already go via your > >> ppc-next queue. > > > >We're missing an ACK in patch 6/13: > > > >hw/ppc/mpc8544ds: Add platform bus > > Bin: Ping? > Sorry for the delay. I have provided the R-b to this patch. Regards, Bin
Re: [PATCH v2 06/13] hw/ppc/mpc8544ds: Add platform bus
On Tue, Oct 4, 2022 at 5:22 AM Bernhard Beschow wrote: > > Models the real device more closely. > > Address and size values are taken from mpc8544.dts from the linux-5.17.7 > tree. The IRQ range is taken from e500plat.c. > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/mpc8544ds.c | 6 ++ > 1 file changed, 6 insertions(+) > Reviewed-by: Bin Meng
Re: [PATCH 04/11] hw/ppc/mpc8544ds: Add platform bus
Hi Bernhard, On Sat, Sep 17, 2022 at 1:19 AM Bernhard Beschow wrote: > > Am 16. September 2022 06:15:53 UTC schrieb Bin Meng : > >On Thu, Sep 15, 2022 at 11:29 PM Bernhard Beschow wrote: > >> > >> Models the real device more closely. > > > >Please describe the source (e.g.: I assume it's MPC8544DS board manual > >or something like that?) that describe such memory map for the > >platform bus. > > > >Is this the eLBC bus range that includes the NOR flash device? > > Good point. My numbers come from a different board. I'll fix them according > to the mpc8544ds.dts in the Linux tree. > > This will leave an eLBC memory window of just 8MB while my proprietary load > needs 64MB. My proprietary load doesn't seem to have 64 bit physical memory > support so I can't use e500plat either. Any suggestions? > Currently QEMU does not model the eLBC registers so these memory regions have to be hardcoded, unfortunately. Once we support eLBC memory map completely I think we can remove such limitations by having QEMU dynamically create the memory map per programmed values. I guess you have to create another machine for your board at this point. Regards, Bin
Re: [PATCH v2 05/13] hw/ppc/mpc8544ds: Rename wrongly named method
On Tue, Oct 4, 2022 at 5:15 AM Bernhard Beschow wrote: > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/mpc8544ds.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Bin Meng
Re: [PATCH v2 04/13] hw/ppc/e500: Reduce usage of sysbus API
On Tue, Oct 4, 2022 at 5:24 AM Bernhard Beschow wrote: > > PlatformBusDevice has an mmio attribute which gets aliased to > SysBusDevice::mmio[0]. So PlatformbusDevice::mmio can be used directly, > avoiding the sysbus API. > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/e500.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Bin Meng
Re: [PATCH v5 00/18] tests/qtest: Enable running qtest on Windows
On Fri, Oct 7, 2022 at 4:35 AM Alex Bennée wrote: > > > Bin Meng writes: > > > In preparation to adding virtio-9p support on Windows, this series > > enables running qtest on Windows, so that we can run the virtio-9p > > tests on Windows to make sure it does not break accidently. > > I'm happy to take this whole series through my testing/next however I > don't have working CI for the month so need to wait for my minutes to > reset. Have you done a full CI run* with this? > Yes, gitlab CI passed. > (*make sure any CI run is only on a repo forked from > https://gitlab.com/qemu-project as you won't get the discount cost > factor otherwise) > Regards, Bin
[PATCH v5 05/18] block/vvfat: Unify the mkdir() call
From: Bin Meng
There is a difference in the mkdir() call for win32 and non-win32
platforms, and currently is handled in the codes with #ifdefs.
glib provides a portable g_mkdir() API and we can use it to unify
the codes without #ifdefs.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v2)
Changes in v2:
- Change to use g_mkdir()
block/vvfat.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..723beef025 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include
+#include
#include "qapi/error.h"
#include "block/block_int.h"
#include "block/qdict.h"
@@ -2726,13 +2727,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
mapping_t* mapping;
int j, parent_path_len;
-#ifdef __MINGW32__
-if (mkdir(commit->path))
+if (g_mkdir(commit->path, 0755)) {
return -5;
-#else
-if (mkdir(commit->path, 0755))
-return -5;
-#endif
+}
mapping = insert_mapping(s, commit->param.mkdir.cluster,
commit->param.mkdir.cluster + 1);
--
2.34.1
[PATCH v5 00/18] tests/qtest: Enable running qtest on Windows
In preparation to adding virtio-9p support on Windows, this series enables running qtest on Windows, so that we can run the virtio-9p tests on Windows to make sure it does not break accidently. Changes in v5: - Rebase on qemu/master - Drop patches that are already merged - Use g_autoptr(GError) - Restore to v1 version which does not touch the posix implementation - Replace sighandler_t with its actual definition, since it is not available on BSD hosts Changes in v4: - Update the error reporting by using the GError "error" argument of g_dir_make_tmp() - Remove the const from tmpfs declaration Changes in v3: - Split to a separate patch - Add a usleep(1) in the busy wait loop - Drop the host test Changes in v2: - Use g_autofree to declare the variable - Change to use g_mkdir() - Change to use g_mkdir() - Change to use g_mkdir() - Introduce qemu_send_full() and use it - Move the enabling of building qtests on Windows to a separate patch to keep bisectablity - Call socket_init() unconditionally - Add a missing CloseHandle() call - Change to a busy wait after migration is canceled - new patch: "io/channel-watch: Drop the unnecessary cast" - Change the timeout limit to 90 minutes - new patch: "tests/qtest: Enable qtest build on Windows" Bin Meng (15): semihosting/arm-compat-semi: Avoid using hardcoded /tmp tcg: Avoid using hardcoded /tmp util/qemu-sockets: Use g_get_tmp_dir() to get the directory for temporary files tests/qtest: migration-test: Avoid using hardcoded /tmp block/vvfat: Unify the mkdir() call fsdev/virtfs-proxy-helper: Use g_mkdir() hw/usb: dev-mtp: Use g_mkdir() tests/qtest: libqtest: Install signal handler via signal() tests/qtest: Support libqtest to build and run on Windows tests/qtest: libqtest: Correct the timeout unit of blocking receive calls for win32 io/channel-watch: Drop a superfluous '#ifdef WIN32' io/channel-watch: Drop the unnecessary cast io/channel-watch: Fix socket watch on Windows .gitlab-ci.d/windows.yml: Increase the timeout to 90 minutes tests/qtest: Enable qtest build on Windows Xuzhou Cheng (3): accel/qtest: Support qtest accelerator for Windows tests/qtest: Use send/recv for socket communication tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled include/hw/core/cpu.h | 1 + include/qemu/sockets.h| 2 + accel/dummy-cpus.c| 14 +++- block/vvfat.c | 9 +-- fsdev/virtfs-proxy-helper.c | 3 +- hw/usb/dev-mtp.c | 4 +- io/channel-watch.c| 12 +--- semihosting/arm-compat-semi.c | 3 +- softmmu/cpus.c| 9 +-- tcg/tcg.c | 3 +- tests/qtest/libqmp.c | 5 +- tests/qtest/libqtest.c| 124 +- tests/qtest/migration-test.c | 14 ++-- util/osdep.c | 33 + util/qemu-sockets.c | 5 +- .gitlab-ci.d/windows.yml | 4 +- accel/meson.build | 1 + accel/qtest/meson.build | 1 + tests/qtest/meson.build | 6 -- 19 files changed, 194 insertions(+), 59 deletions(-) -- 2.34.1
Re: [PATCH 00/18] tests/qtest: Enable running qtest on Windows
On Thu, Oct 6, 2022 at 11:11 PM Bin Meng wrote: > > In preparation to adding virtio-9p support on Windows, this series > enables running qtest on Windows, so that we can run the virtio-9p > tests on Windows to make sure it does not break accidently. > > Changes in v5: > - Rebase on qemu/master > - Drop patches that are already merged > - Use g_autoptr(GError) > - Restore to v1 version which does not touch the posix implementation > - Replace sighandler_t with its actual definition, since it is not > available on BSD hosts > Sorry, forgot to add the version PREFIX. Will resend. Regards, Bin
[PATCH 05/18] block/vvfat: Unify the mkdir() call
From: Bin Meng
There is a difference in the mkdir() call for win32 and non-win32
platforms, and currently is handled in the codes with #ifdefs.
glib provides a portable g_mkdir() API and we can use it to unify
the codes without #ifdefs.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v2)
Changes in v2:
- Change to use g_mkdir()
block/vvfat.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..723beef025 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include
+#include
#include "qapi/error.h"
#include "block/block_int.h"
#include "block/qdict.h"
@@ -2726,13 +2727,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
mapping_t* mapping;
int j, parent_path_len;
-#ifdef __MINGW32__
-if (mkdir(commit->path))
+if (g_mkdir(commit->path, 0755)) {
return -5;
-#else
-if (mkdir(commit->path, 0755))
-return -5;
-#endif
+}
mapping = insert_mapping(s, commit->param.mkdir.cluster,
commit->param.mkdir.cluster + 1);
--
2.34.1
[PATCH 00/18] tests/qtest: Enable running qtest on Windows
In preparation to adding virtio-9p support on Windows, this series enables running qtest on Windows, so that we can run the virtio-9p tests on Windows to make sure it does not break accidently. Changes in v5: - Rebase on qemu/master - Drop patches that are already merged - Use g_autoptr(GError) - Restore to v1 version which does not touch the posix implementation - Replace sighandler_t with its actual definition, since it is not available on BSD hosts Changes in v4: - Update the error reporting by using the GError "error" argument of g_dir_make_tmp() - Remove the const from tmpfs declaration Changes in v3: - Split to a separate patch - Add a usleep(1) in the busy wait loop - Drop the host test Changes in v2: - Use g_autofree to declare the variable - Change to use g_mkdir() - Change to use g_mkdir() - Change to use g_mkdir() - Introduce qemu_send_full() and use it - Move the enabling of building qtests on Windows to a separate patch to keep bisectablity - Call socket_init() unconditionally - Add a missing CloseHandle() call - Change to a busy wait after migration is canceled - new patch: "io/channel-watch: Drop the unnecessary cast" - Change the timeout limit to 90 minutes - new patch: "tests/qtest: Enable qtest build on Windows" Bin Meng (15): semihosting/arm-compat-semi: Avoid using hardcoded /tmp tcg: Avoid using hardcoded /tmp util/qemu-sockets: Use g_get_tmp_dir() to get the directory for temporary files tests/qtest: migration-test: Avoid using hardcoded /tmp block/vvfat: Unify the mkdir() call fsdev/virtfs-proxy-helper: Use g_mkdir() hw/usb: dev-mtp: Use g_mkdir() tests/qtest: libqtest: Install signal handler via signal() tests/qtest: Support libqtest to build and run on Windows tests/qtest: libqtest: Correct the timeout unit of blocking receive calls for win32 io/channel-watch: Drop a superfluous '#ifdef WIN32' io/channel-watch: Drop the unnecessary cast io/channel-watch: Fix socket watch on Windows .gitlab-ci.d/windows.yml: Increase the timeout to 90 minutes tests/qtest: Enable qtest build on Windows Xuzhou Cheng (3): accel/qtest: Support qtest accelerator for Windows tests/qtest: Use send/recv for socket communication tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled include/hw/core/cpu.h | 1 + include/qemu/sockets.h| 2 + accel/dummy-cpus.c| 14 +++- block/vvfat.c | 9 +-- fsdev/virtfs-proxy-helper.c | 3 +- hw/usb/dev-mtp.c | 4 +- io/channel-watch.c| 12 +--- semihosting/arm-compat-semi.c | 3 +- softmmu/cpus.c| 9 +-- tcg/tcg.c | 3 +- tests/qtest/libqmp.c | 5 +- tests/qtest/libqtest.c| 124 +- tests/qtest/migration-test.c | 14 ++-- util/osdep.c | 33 + util/qemu-sockets.c | 5 +- .gitlab-ci.d/windows.yml | 4 +- accel/meson.build | 1 + accel/qtest/meson.build | 1 + tests/qtest/meson.build | 6 -- 19 files changed, 194 insertions(+), 59 deletions(-) -- 2.34.1
Re: [PATCH v4 00/54] tests/qtest: Enable running qtest on Windows
Hi Marc-André, On Mon, Oct 3, 2022 at 5:26 PM Marc-André Lureau wrote: > > Hi Bin > > On Tue, Sep 27, 2022 at 3:18 PM Bin Meng wrote: >> >> In preparation to adding virtio-9p support on Windows, this series >> enables running qtest on Windows, so that we can run the virtio-9p >> tests on Windows to make sure it does not break accidently. >> >> Changes in v4: >> - Do not use g_autofree and g_steal_pointer >> - Update the error reporting by using the GError "error" argument >> of g_dir_make_tmp() >> - Remove the const from tmpfs declaration >> - Replace the whole block with a g_assert_no_error() >> - Replace the error reporting with g_assert_no_error() >> - Update error reporting >> - Move the new text section after the "QTest" section instead >> - Use plural in both cases: "on POSIX hosts as well as Windows hosts" >> - Use "The following list shows some best practices" >> - Fix typo of delimiter >> - New patch: "tests/qtest: boot-serial-test: Close the serial file before >> starting QEMU" >> - Drop patch: "chardev/char-file: Add FILE_SHARE_WRITE when openning the >> file for win32" >> > > Could you post a v5 rebased on the current master? thanks > Sure, will do. > (I think most of the remaining patches are simple enough that I could take > them in a misc PR if they are not picked by subsystem maintainers) Thank you. Regards, Bin
Re: [PATCH v5 2/2] block: Refactor get_tmp_filename()
Hi Kevin,
On Fri, Sep 30, 2022 at 6:13 PM Kevin Wolf wrote:
>
> Am 28.09.2022 um 16:41 hat Bin Meng geschrieben:
> > From: Bin Meng
> >
> > At present there are two callers of get_tmp_filename() and they are
> > inconsistent.
> >
> > One does:
> >
> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
> > ...
> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> >
> > while the other does:
> >
> > s->qcow_filename = g_malloc(PATH_MAX);
> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> >
> > As we can see different 'size' arguments are passed. There are also
> > platform specific implementations inside the function, and the use
> > of snprintf is really undesirable.
> >
> > The function name is also misleading. It creates a temporary file,
> > not just a filename.
> >
> > Refactor this routine by changing its name and signature to:
> >
> > char *create_tmp_file(Error **errp)
> >
> > and use g_file_open_tmp() for a consistent implementation.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > Changes in v5:
> > - minor change in the commit message
> > - add some notes in the function comment block
> > - add g_autofree for tmp_filename
> >
> > Changes in v4:
> > - Rename the function to create_tmp_file() and take "Error **errp" as
> > a parameter, so that callers can pass errp all the way down to this
> > routine.
> > - Commit message updated to reflect the latest change
> >
> > Changes in v3:
> > - Do not use errno directly, instead still let get_tmp_filename() return
> > a negative number to indicate error
> >
> > Changes in v2:
> > - Use g_autofree and g_steal_pointer
> >
> > include/block/block_int-common.h | 2 +-
> > block.c | 45
> > block/vvfat.c| 7 +++--
> > 3 files changed, 20 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/block/block_int-common.h
> > b/include/block/block_int-common.h
> > index 8947abab76..d7c0a7e96f 100644
> > --- a/include/block/block_int-common.h
> > +++ b/include/block/block_int-common.h
> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> > *child)
> > }
> >
> > int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> > -int get_tmp_filename(char *filename, int size);
> > +char *create_tmp_file(Error **errp);
> > void bdrv_parse_filename_strip_prefix(const char *filename, const char
> > *prefix,
> >QDict *options);
> >
> > diff --git a/block.c b/block.c
> > index 582c205307..bd3006d85d 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -860,35 +860,25 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> > HDGeometry *geo)
> >
> > /*
> > * Create a uniquely-named empty temporary file.
> > - * Return 0 upon success, otherwise a negative errno value.
> > + * Return the actual file name used upon success, otherwise NULL.
> > + * This string should be freed with g_free() when not needed any longer.
> > + *
> > + * Note: creating a temporary file for the caller to (re)open is
> > + * inherently racy. Use g_file_open_tmp() instead whenever practical.
> > */
> > -int get_tmp_filename(char *filename, int size)
> > +char *create_tmp_file(Error **errp)
> > {
> > -#ifdef _WIN32
> > -char temp_dir[MAX_PATH];
> > -/* GetTempFileName requires that its output buffer (4th param)
> > - have length MAX_PATH or greater. */
> > -assert(size >= MAX_PATH);
> > -return (GetTempPath(MAX_PATH, temp_dir)
> > -&& GetTempFileName(temp_dir, "qem", 0, filename)
>
> We're using different prefixes on Windows and on Linux. This patch
> unifies both paths to use the Linux name. Nobody should rely on the name
> of temporary files, so there is hope it won't break anything.
>
> > -? 0 : -GetLastError());
> > -#else
> > +g_autofree char *name = NULL;
> > +g_autoptr(GError) err = NULL;
> > int fd;
> > -const char *tmpdir;
> > -tmpdir = getenv("TMPDIR");
> > -if (!tmpdir) {
> > -tmpdir = "/var/tmp";
> > -}
> > -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> > -return
Re: [PATCH v4 25/54] block/vvfat: Unify the mkdir() call
Hi Kevin, On Tue, Sep 27, 2022 at 7:07 PM Bin Meng wrote: > > From: Bin Meng > > There is a difference in the mkdir() call for win32 and non-win32 > platforms, and currently is handled in the codes with #ifdefs. > > glib provides a portable g_mkdir() API and we can use it to unify > the codes without #ifdefs. > > Signed-off-by: Bin Meng > Reviewed-by: Marc-André Lureau > --- > > (no changes since v2) > > Changes in v2: > - Change to use g_mkdir() > > block/vvfat.c | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) > Would you pick up this patch in your queue? Regards, Bin
[PATCH v5 2/2] block: Refactor get_tmp_filename()
From: Bin Meng
At present there are two callers of get_tmp_filename() and they are
inconsistent.
One does:
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char *tmp_filename = g_malloc0(PATH_MAX + 1);
...
ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
while the other does:
s->qcow_filename = g_malloc(PATH_MAX);
ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
As we can see different 'size' arguments are passed. There are also
platform specific implementations inside the function, and the use
of snprintf is really undesirable.
The function name is also misleading. It creates a temporary file,
not just a filename.
Refactor this routine by changing its name and signature to:
char *create_tmp_file(Error **errp)
and use g_file_open_tmp() for a consistent implementation.
Signed-off-by: Bin Meng
---
Changes in v5:
- minor change in the commit message
- add some notes in the function comment block
- add g_autofree for tmp_filename
Changes in v4:
- Rename the function to create_tmp_file() and take "Error **errp" as
a parameter, so that callers can pass errp all the way down to this
routine.
- Commit message updated to reflect the latest change
Changes in v3:
- Do not use errno directly, instead still let get_tmp_filename() return
a negative number to indicate error
Changes in v2:
- Use g_autofree and g_steal_pointer
include/block/block_int-common.h | 2 +-
block.c | 45
block/vvfat.c| 7 +++--
3 files changed, 20 insertions(+), 34 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..d7c0a7e96f 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild *child)
}
int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
-int get_tmp_filename(char *filename, int size);
+char *create_tmp_file(Error **errp);
void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
QDict *options);
diff --git a/block.c b/block.c
index 582c205307..bd3006d85d 100644
--- a/block.c
+++ b/block.c
@@ -860,35 +860,25 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
/*
* Create a uniquely-named empty temporary file.
- * Return 0 upon success, otherwise a negative errno value.
+ * Return the actual file name used upon success, otherwise NULL.
+ * This string should be freed with g_free() when not needed any longer.
+ *
+ * Note: creating a temporary file for the caller to (re)open is
+ * inherently racy. Use g_file_open_tmp() instead whenever practical.
*/
-int get_tmp_filename(char *filename, int size)
+char *create_tmp_file(Error **errp)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
+g_autofree char *name = NULL;
+g_autoptr(GError) err = NULL;
int fd;
-const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
-if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
-return -EOVERFLOW;
-}
-fd = mkstemp(filename);
+
+fd = g_file_open_tmp("vl.XX", , );
if (fd < 0) {
-return -errno;
+error_setg(errp, "%s", err->message);
+return NULL;
}
close(fd);
-return 0;
-#endif
+return g_steal_pointer();
}
/*
@@ -3714,8 +3704,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
QDict *snapshot_options,
Error **errp)
{
-/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
-char *tmp_filename = g_malloc0(PATH_MAX + 1);
+g_autofree char *tmp_filename = NULL;
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
@@ -3734,9 +3723,8 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
}
/* Create the temporary image */
-ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not get temporary filename");
+tmp_filename = create_tmp_file(errp);
+if (!tmp_filename) {
goto out;
}
@@ -3770,7 +3758,6 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
out:
qobject_unref(snapshot_options);
-g_free(tmp_filename);
return bs_snapshot;
}
d
[PATCH v5 1/2] block: Ignore close() failure in get_tmp_filename()
From: Bin Meng
The temporary file has been created and is ready for use. Checking
return value of close() does not seem useful. The file descriptor
is almost certainly closed; see close(2) under "Dealing with error
returns from close()".
Let's simply ignore close() failure here.
Suggested-by: Markus Armbruster
Signed-off-by: Bin Meng
---
Changes in v5:
- new patch: "block: Ignore close() failure in get_tmp_filename()"
block.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/block.c b/block.c
index bc85f46eed..582c205307 100644
--- a/block.c
+++ b/block.c
@@ -886,10 +886,7 @@ int get_tmp_filename(char *filename, int size)
if (fd < 0) {
return -errno;
}
-if (close(fd) != 0) {
-unlink(filename);
-return -errno;
-}
+close(fd);
return 0;
#endif
}
--
2.34.1
[PATCH v4] block: Refactor get_tmp_filename()
From: Bin Meng
At present there are two callers of get_tmp_filename() and they are
inconsistent.
One does:
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char *tmp_filename = g_malloc0(PATH_MAX + 1);
...
ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
while the other does:
s->qcow_filename = g_malloc(PATH_MAX);
ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
As we can see different 'size' arguments are passed. There are also
platform specific implementations inside the function, and this use
of snprintf is really undesirable.
The function name is also misleading. It creates a temporary file,
not just a filename.
Refactor this routine by changing its name and signature to:
char *create_tmp_file(Error **errp)
and use g_file_open_tmp() for a consistent implementation.
Signed-off-by: Bin Meng
---
Changes in v4:
- Rename the function to create_tmp_file() and take "Error **errp" as
a parameter, so that callers can pass errp all the way down to this
routine.
- Commit message updated to reflect the latest change
Changes in v3:
- Do not use errno directly, instead still let get_tmp_filename() return
a negative number to indicate error
Changes in v2:
- Use g_autofree and g_steal_pointer
include/block/block_int-common.h | 2 +-
block.c | 47
block/vvfat.c| 7 ++---
3 files changed, 21 insertions(+), 35 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..d7c0a7e96f 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild *child)
}
int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
-int get_tmp_filename(char *filename, int size);
+char *create_tmp_file(Error **errp);
void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
QDict *options);
diff --git a/block.c b/block.c
index bc85f46eed..b33bd774ae 100644
--- a/block.c
+++ b/block.c
@@ -860,38 +860,27 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
/*
* Create a uniquely-named empty temporary file.
- * Return 0 upon success, otherwise a negative errno value.
+ * Return the actual file name used upon success, otherwise NULL.
+ * This string should be freed with g_free() when not needed any longer.
*/
-int get_tmp_filename(char *filename, int size)
+char *create_tmp_file(Error **errp)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
+g_autofree char *name = NULL;
+g_autoptr(GError) err = NULL;
int fd;
-const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
-if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
-return -EOVERFLOW;
-}
-fd = mkstemp(filename);
+
+fd = g_file_open_tmp("vl.XX", , );
if (fd < 0) {
-return -errno;
+error_setg_errno(errp, -ENOENT, "%s", err->message);
+return NULL;
}
if (close(fd) != 0) {
-unlink(filename);
-return -errno;
+unlink(name);
+error_setg_errno(errp, -errno, "Could not close the temporary file");
+return NULL;
}
-return 0;
-#endif
+
+return g_steal_pointer();
}
/*
@@ -3717,8 +3706,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
QDict *snapshot_options,
Error **errp)
{
-/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
-char *tmp_filename = g_malloc0(PATH_MAX + 1);
+char *tmp_filename = NULL;
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
@@ -3737,9 +3725,8 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
}
/* Create the temporary image */
-ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not get temporary filename");
+tmp_filename = create_tmp_file(errp);
+if (!tmp_filename) {
goto out;
}
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..f9bf8406d3 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3146,10 +3146,9 @@ static int enable_write_target(BlockDriverState *bs,
Error **errp)
array_init(&(s->commits), sizeof(commi
[PATCH v4 40/54] tests/qtest: ide-test: Open file in binary mode
From: Xuzhou Cheng By default Windows opens file in text mode, while a POSIX compliant implementation treats text files and binary files the same. The fopen() 'mode' string can include the letter 'b' to indicate binary mode shall be used. POSIX spec says the character 'b' shall have no effect, but is allowed for ISO C standard conformance. Let's add the letter 'b' which works on both POSIX and Windows. Signed-off-by: Xuzhou Cheng Signed-off-by: Bin Meng Reviewed-by: Marc-André Lureau --- (no changes since v2) Changes in v2: - Drop ahci-test.c changes that are no longer needed tests/qtest/ide-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c index 5e3e28aea2..4ea89c26c9 100644 --- a/tests/qtest/ide-test.c +++ b/tests/qtest/ide-test.c @@ -892,7 +892,7 @@ static void cdrom_pio_impl(int nblocks) /* Prepopulate the CDROM with an interesting pattern */ generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh); g_assert_cmpint(ret, ==, patt_blocks); fclose(fh); @@ -993,7 +993,7 @@ static void test_cdrom_dma(void) prdt[0].size = cpu_to_le32(len | PRDT_EOT); generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh); g_assert_cmpint(ret, ==, 16); fclose(fh); -- 2.34.1
[PATCH v4 37/54] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
From: Bin Meng
These test cases uses "blkdebug:path/to/config:path/to/image" for
testing. On Windows, absolute file paths contain the delimiter ':'
which causes the blkdebug filename parser fail to parse filenames.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
Reviewed-by: Thomas Huth
---
(no changes since v1)
tests/qtest/ahci-test.c | 21 ++---
tests/qtest/ide-test.c | 20 ++--
2 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 1d5929d8c3..66652fed04 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type, enum
AddrMode addr,
int main(int argc, char **argv)
{
-const char *arch;
+const char *arch, *base;
int ret;
int fd;
int c;
@@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
return 0;
}
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create a temporary image */
-fd = g_file_open_tmp("qtest.XX", _path, NULL);
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
+fd = g_mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
imgfmt = "qcow2";
@@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
+fd = g_mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 25302be6dc..5e3e28aea2 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -1011,16 +1011,32 @@ static void test_cdrom_dma(void)
int main(int argc, char **argv)
{
+const char *base;
int fd;
int ret;
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create temporary blkdebug instructions */
-fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
+fd = g_mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
/* Create a temporary raw image */
-fd = g_file_open_tmp("qtest.XX", _path, NULL);
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
+fd = g_mkstemp(tmp_path);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert(ret == 0);
--
2.34.1
[PATCH v4 13/54] tests/qtest: ide-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v3)
Changes in v3:
- Split to a separate patch
tests/qtest/ide-test.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 5bcb75a7e5..25302be6dc 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -121,8 +121,8 @@ enum {
static QPCIBus *pcibus = NULL;
static QGuestAllocator guest_malloc;
-static char tmp_path[] = "/tmp/qtest.XX";
-static char debug_path[] = "/tmp/qtest-blkdebug.XX";
+static char *tmp_path;
+static char *debug_path;
static QTestState *ide_test_start(const char *cmdline_fmt, ...)
{
@@ -1015,12 +1015,12 @@ int main(int argc, char **argv)
int ret;
/* Create temporary blkdebug instructions */
-fd = mkstemp(debug_path);
+fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
g_assert(fd >= 0);
close(fd);
/* Create a temporary raw image */
-fd = mkstemp(tmp_path);
+fd = g_file_open_tmp("qtest.XX", _path, NULL);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert(ret == 0);
@@ -1049,7 +1049,9 @@ int main(int argc, char **argv)
/* Cleanup */
unlink(tmp_path);
+g_free(tmp_path);
unlink(debug_path);
+g_free(debug_path);
return ret;
}
--
2.34.1
[PATCH v4 19/54] tests/qtest: virtio-blk-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v3)
Changes in v3:
- Split to a separate patch
tests/qtest/virtio-blk-test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c
index dc5eed31c8..19c01f808b 100644
--- a/tests/qtest/virtio-blk-test.c
+++ b/tests/qtest/virtio-blk-test.c
@@ -49,10 +49,10 @@ static void drive_destroy(void *path)
static char *drive_create(void)
{
int fd, ret;
-char *t_path = g_strdup("/tmp/qtest.XX");
+char *t_path;
/* Create a temporary raw image */
-fd = mkstemp(t_path);
+fd = g_file_open_tmp("qtest.XX", _path, NULL);
g_assert_cmpint(fd, >=, 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert_cmpint(ret, ==, 0);
--
2.34.1
[PATCH v4 25/54] block/vvfat: Unify the mkdir() call
From: Bin Meng
There is a difference in the mkdir() call for win32 and non-win32
platforms, and currently is handled in the codes with #ifdefs.
glib provides a portable g_mkdir() API and we can use it to unify
the codes without #ifdefs.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v2)
Changes in v2:
- Change to use g_mkdir()
block/vvfat.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..723beef025 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include
+#include
#include "qapi/error.h"
#include "block/block_int.h"
#include "block/qdict.h"
@@ -2726,13 +2727,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
mapping_t* mapping;
int j, parent_path_len;
-#ifdef __MINGW32__
-if (mkdir(commit->path))
+if (g_mkdir(commit->path, 0755)) {
return -5;
-#else
-if (mkdir(commit->path, 0755))
-return -5;
-#endif
+}
mapping = insert_mapping(s, commit->param.mkdir.cluster,
commit->param.mkdir.cluster + 1);
--
2.34.1
[PATCH v4 00/54] tests/qtest: Enable running qtest on Windows
In preparation to adding virtio-9p support on Windows, this series enables running qtest on Windows, so that we can run the virtio-9p tests on Windows to make sure it does not break accidently. Changes in v4: - Do not use g_autofree and g_steal_pointer - Update the error reporting by using the GError "error" argument of g_dir_make_tmp() - Remove the const from tmpfs declaration - Replace the whole block with a g_assert_no_error() - Replace the error reporting with g_assert_no_error() - Update error reporting - Move the new text section after the "QTest" section instead - Use plural in both cases: "on POSIX hosts as well as Windows hosts" - Use "The following list shows some best practices" - Fix typo of delimiter - New patch: "tests/qtest: boot-serial-test: Close the serial file before starting QEMU" - Drop patch: "chardev/char-file: Add FILE_SHARE_WRITE when openning the file for win32" Changes in v3: - Remove unnecessary "error = NULL" statements - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Split to a separate patch - Split to a separate patch - Split to a separate patch - Ensure g_autofree variable is initialized - Use g_steal_pointer() in create_test_img() - Split to a separate patch - Split to a separate patch - Split to a separate patch - Split to a separate patch - Split to a separate patch - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Ensure g_autofree variable is initialized - Split to a separate patch - Split to a separate patch - Add a usleep(1) in the busy wait loop - Drop the host test - Drop patch: "tests: Change to use g_mkdir()" - Drop patch: "block: Unify the get_tmp_filename() implementation", and send it as a separate patch Changes in v2: - new patch: "tests/qtest: i440fx-test: Rewrite create_blob_file() to be portable" - Use g_autofree to declare the variable - Change to use g_mkdir() - Change to use g_mkdir() - Change to use g_mkdir() - Change to skip only part of the virtio-net-test cases that require socketpair() intead of disabling all of them - Introduce a new variable qtests_filter and add that to the qtests_ARCH variables - Add a comment in the code to explain why test_qmp_oob test case is skipped on win32 - Replace signal by the semaphore on posix too - Use __declspec(selectany) for the common weak symbol on Windows - Introduce qemu_send_full() and use it - Move the enabling of building qtests on Windows to a separate patch to keep bisectablity - Call socket_init() unconditionally - Add a missing CloseHandle() call - Drop ahci-test.c changes that are no longer needed - Change the place that sets IO redirection in the command line - Change to a busy wait after migration is canceled - new patch: "io/channel-watch: Drop the unnecessary cast" - Change the timeout limit to 90 minutes - new patch: Display meson test logs in the Windows CI - new patch: "tests/qtest: Enable qtest build on Windows" - Minor wording changes - Drop patches that were already applied in the mainline - Drop patch: "qga/commands-posix-ssh: Use g_mkdir_with_parents()" - Drop patch: "tests: Skip iotests and qtest when '--without-default-devices'" - Drop patch: "tests/qtest: Fix ERROR_SHARING_VIOLATION for win32" Bin Meng (48): tests/qtest: i440fx-test: Rewrite create_blob_file() to be portable semihosting/arm-compat-semi: Avoid using hardcoded /tmp tcg: Avoid using hardcoded /tmp util/qemu-sockets: Use g_get_tmp_dir() to get the directory for temporary files tests/qtest: ahci-test: Avoid using hardcoded /tmp tests/qtest: aspeed_smc-test: Avoid using hardcoded /tmp tests/qtest: boot-serial-test: Avoid using hardcoded /tmp tests/qtest: cxl-test: Avoid using hardcoded /tmp tests/qtest: fdc-test: Avoid using hardcoded /tmp tests/qtest: generic_fuzz: Avoid using hardcoded /tmp tests/qtest: virtio_blk_fuzz: Avoid using hardcoded /tmp tests/qtest: hd-geo-test: Avoid using hardcoded /tmp tests/qtest: ide-test: Avoid using hardcoded /tmp tests/qtest: migration-test: Avoid using hardcoded /tmp tests/qtest: pflash-cfi02-test: Avoid using hardcoded /tmp tests/qtest: qmp-test: Avoid using hardcoded /tmp tests/qtest: vhost-user-blk-test: Avoid using hardcoded /tmp tests/qtest: vhost-user-test: Avoid using hardcoded /tmp tests/qtest: virtio-blk-test: Avoid using hardcoded /tmp tests/qtest: virtio-scsi-test: Avoid using hardcod
[PATCH v4 09/54] tests/qtest: fdc-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v3)
Changes in v3:
- Split to a separate patch
tests/qtest/fdc-test.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 52ade90a7d..1f9b99ad6d 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -68,7 +68,7 @@ enum {
DSKCHG = 0x80,
};
-static char test_image[] = "/tmp/qtest.XX";
+static char *test_image;
#define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==, (mask))
#define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==, 0)
@@ -608,7 +608,7 @@ int main(int argc, char **argv)
int ret;
/* Create a temporary raw image */
-fd = mkstemp(test_image);
+fd = g_file_open_tmp("qtest.XX", _image, NULL);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert(ret == 0);
@@ -640,6 +640,7 @@ int main(int argc, char **argv)
/* Cleanup */
qtest_end();
unlink(test_image);
+g_free(test_image);
return ret;
}
--
2.34.1
[PATCH v4 05/54] tests/qtest: ahci-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
Reviewed-by: Thomas Huth
---
(no changes since v3)
Changes in v3:
- Split to a separate patch
- Ensure g_autofree variable is initialized
tests/qtest/ahci-test.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index f1e510b0ac..1d5929d8c3 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -44,9 +44,9 @@
#define TEST_IMAGE_SIZE_MB_SMALL 64
/*** Globals ***/
-static char tmp_path[] = "/tmp/qtest.XX";
-static char debug_path[] = "/tmp/qtest-blkdebug.XX";
-static char mig_socket[] = "/tmp/qtest-migration.XX";
+static char *tmp_path;
+static char *debug_path;
+static char *mig_socket;
static bool ahci_pedantic;
static const char *imgfmt;
static unsigned test_image_size_mb;
@@ -1437,10 +1437,10 @@ static void test_ncq_simple(void)
static int prepare_iso(size_t size, unsigned char **buf, char **name)
{
-char cdrom_path[] = "/tmp/qtest.iso.XX";
+g_autofree char *cdrom_path = NULL;
unsigned char *patt;
ssize_t ret;
-int fd = mkstemp(cdrom_path);
+int fd = g_file_open_tmp("qtest.iso.XX", _path, NULL);
g_assert(fd != -1);
g_assert(buf);
@@ -1872,7 +1872,7 @@ int main(int argc, char **argv)
}
/* Create a temporary image */
-fd = mkstemp(tmp_path);
+fd = g_file_open_tmp("qtest.XX", _path, NULL);
g_assert(fd >= 0);
if (have_qemu_img()) {
imgfmt = "qcow2";
@@ -1889,12 +1889,12 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-fd = mkstemp(debug_path);
+fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
g_assert(fd >= 0);
close(fd);
/* Reserve a hollow file to use as a socket for migration tests */
-fd = mkstemp(mig_socket);
+fd = g_file_open_tmp("qtest-migration.XX", _socket, NULL);
g_assert(fd >= 0);
close(fd);
@@ -1947,8 +1947,11 @@ int main(int argc, char **argv)
/* Cleanup */
unlink(tmp_path);
+g_free(tmp_path);
unlink(debug_path);
+g_free(debug_path);
unlink(mig_socket);
+g_free(mig_socket);
return ret;
}
--
2.34.1
Re: [PATCH v2] block: Refactor get_tmp_filename()
Hi Markus,
On Tue, Sep 27, 2022 at 2:22 PM Markus Armbruster wrote:
>
> Bin Meng writes:
>
> > On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster wrote:
> >>
> >> Bin Meng writes:
> >>
> >> > From: Bin Meng
> >> >
> >> > At present there are two callers of get_tmp_filename() and they are
> >> > inconsistent.
> >> >
> >> > One does:
> >> >
> >> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> >> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
> >> > ...
> >> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> >> >
> >> > while the other does:
> >> >
> >> > s->qcow_filename = g_malloc(PATH_MAX);
> >> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> >> >
> >> > As we can see different 'size' arguments are passed. There are also
> >> > platform specific implementations inside the function, and this use
> >> > of snprintf is really undesirable.
> >> >
> >> > Refactor this routine by changing its signature to:
> >> >
> >> > char *get_tmp_filename(void)
> >> >
> >> > and use g_file_open_tmp() for a consistent implementation.
> >> >
> >> > Signed-off-by: Bin Meng
> >> > ---
> >> >
> >> > Changes in v2:
> >> > - Use g_autofree and g_steal_pointer
> >> >
> >> > include/block/block_int-common.h | 2 +-
> >> > block.c | 42 ++--
> >> > block/vvfat.c| 8 +++---
> >> > 3 files changed, 18 insertions(+), 34 deletions(-)
> >> >
> >> > diff --git a/include/block/block_int-common.h
> >> > b/include/block/block_int-common.h
> >> > index 8947abab76..ea69a9349c 100644
> >> > --- a/include/block/block_int-common.h
> >> > +++ b/include/block/block_int-common.h
> >> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> >> > *child)
> >> > }
> >> >
> >> > int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> >> > -int get_tmp_filename(char *filename, int size);
> >> > +char *get_tmp_filename(void);
> >> > void bdrv_parse_filename_strip_prefix(const char *filename, const char
> >> > *prefix,
> >> >QDict *options);
> >> >
> >> > diff --git a/block.c b/block.c
> >> > index bc85f46eed..4e7a795566 100644
> >> > --- a/block.c
> >> > +++ b/block.c
> >> > @@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> >> > HDGeometry *geo)
> >> >
> >> > /*
> >> > * Create a uniquely-named empty temporary file.
> >> > - * Return 0 upon success, otherwise a negative errno value.
> >> > + * Return the actual name used upon success, otherwise NULL.
> >> > + * The called function is responsible for freeing it.
> >> > */
> >> > -int get_tmp_filename(char *filename, int size)
> >> > +char *get_tmp_filename(void)
> >> > {
> >> > -#ifdef _WIN32
> >> > -char temp_dir[MAX_PATH];
> >> > -/* GetTempFileName requires that its output buffer (4th param)
> >> > - have length MAX_PATH or greater. */
> >> > -assert(size >= MAX_PATH);
> >> > -return (GetTempPath(MAX_PATH, temp_dir)
> >> > -&& GetTempFileName(temp_dir, "qem", 0, filename)
> >> > -? 0 : -GetLastError());
> >> > -#else
> >> > +g_autofree char *filename = NULL;
> >> > int fd;
> >> > -const char *tmpdir;
> >> > -tmpdir = getenv("TMPDIR");
> >> > -if (!tmpdir) {
> >> > -tmpdir = "/var/tmp";
> >> > -}
> >> > -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> >> > -return -EOVERFLOW;
> >> > -}
> >> > -fd = mkstemp(filename);
> >> > +
> >> > +fd = g_file_open_tmp("vl.XX", , NULL);
> >> > if (fd < 0) {
> >> > -return -errno;
> >> > +return NULL;
> >> > }
> >>
Re: [PATCH v3 37/54] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
Hi Thomas,
On Tue, Sep 27, 2022 at 12:20 AM Thomas Huth wrote:
>
> On 25/09/2022 13.30, Bin Meng wrote:
> > From: Bin Meng
> >
> > These test cases uses "blkdebug:path/to/config:path/to/image" for
> > testing. On Windows, absolute file paths contain the delimiter ':'
> > which causes the blkdebug filename parser fail to parse filenames.
> >
> > Signed-off-by: Bin Meng
> > Reviewed-by: Marc-André Lureau
> > ---
> >
> > (no changes since v1)
> >
> > tests/qtest/ahci-test.c | 21 ++---
> > tests/qtest/ide-test.c | 20 ++--
> > 2 files changed, 36 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> > index 1d5929d8c3..66652fed04 100644
> > --- a/tests/qtest/ahci-test.c
> > +++ b/tests/qtest/ahci-test.c
> > @@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type,
> > enum AddrMode addr,
> >
> > int main(int argc, char **argv)
> > {
> > -const char *arch;
> > +const char *arch, *base;
> > int ret;
> > int fd;
> > int c;
> > @@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
> > return 0;
> > }
> >
> > +/*
> > + * "base" stores the starting point where we create temporary files.
> > + *
> > + * On Windows, this is set to the relative path of current working
> > + * directory, because the absolute path causes the blkdebug filename
> > + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> > + */
> > +#ifndef _WIN32
> > +base = g_get_tmp_dir();
> > +#else
> > +base = ".";
> > +#endif
> > +
> > /* Create a temporary image */
> > -fd = g_file_open_tmp("qtest.XX", _path, NULL);
> > +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> > +fd = g_mkstemp(tmp_path);
> > g_assert(fd >= 0);
> > if (have_qemu_img()) {
> > imgfmt = "qcow2";
> > @@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
> > close(fd);
> >
> > /* Create temporary blkdebug instructions */
> > -fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
> > +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> > +fd = g_mkstemp(debug_path);
> > g_assert(fd >= 0);
> > close(fd);
>
> It would maybe make sense to merge this with patch 05 ("tests/qtest:
> ahci-test: Avoid using hardcoded /tmp") ? ... but if you want to keep it
> separate, that's fine for me, too.
I'd prefer to keep these two patches separate as they are resolving
different issues.
> Reviewed-by: Thomas Huth
>
Thanks for the review!
Regards,
Bin
Regards,
Bin
Re: [PATCH v2] block: Refactor get_tmp_filename()
On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster wrote:
>
> Bin Meng writes:
>
> > From: Bin Meng
> >
> > At present there are two callers of get_tmp_filename() and they are
> > inconsistent.
> >
> > One does:
> >
> > /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> > char *tmp_filename = g_malloc0(PATH_MAX + 1);
> > ...
> > ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> >
> > while the other does:
> >
> > s->qcow_filename = g_malloc(PATH_MAX);
> > ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> >
> > As we can see different 'size' arguments are passed. There are also
> > platform specific implementations inside the function, and this use
> > of snprintf is really undesirable.
> >
> > Refactor this routine by changing its signature to:
> >
> > char *get_tmp_filename(void)
> >
> > and use g_file_open_tmp() for a consistent implementation.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > Changes in v2:
> > - Use g_autofree and g_steal_pointer
> >
> > include/block/block_int-common.h | 2 +-
> > block.c | 42 ++--
> > block/vvfat.c| 8 +++---
> > 3 files changed, 18 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/block/block_int-common.h
> > b/include/block/block_int-common.h
> > index 8947abab76..ea69a9349c 100644
> > --- a/include/block/block_int-common.h
> > +++ b/include/block/block_int-common.h
> > @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> > *child)
> > }
> >
> > int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> > -int get_tmp_filename(char *filename, int size);
> > +char *get_tmp_filename(void);
> > void bdrv_parse_filename_strip_prefix(const char *filename, const char
> > *prefix,
> >QDict *options);
> >
> > diff --git a/block.c b/block.c
> > index bc85f46eed..4e7a795566 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> > HDGeometry *geo)
> >
> > /*
> > * Create a uniquely-named empty temporary file.
> > - * Return 0 upon success, otherwise a negative errno value.
> > + * Return the actual name used upon success, otherwise NULL.
> > + * The called function is responsible for freeing it.
> > */
> > -int get_tmp_filename(char *filename, int size)
> > +char *get_tmp_filename(void)
> > {
> > -#ifdef _WIN32
> > -char temp_dir[MAX_PATH];
> > -/* GetTempFileName requires that its output buffer (4th param)
> > - have length MAX_PATH or greater. */
> > -assert(size >= MAX_PATH);
> > -return (GetTempPath(MAX_PATH, temp_dir)
> > -&& GetTempFileName(temp_dir, "qem", 0, filename)
> > -? 0 : -GetLastError());
> > -#else
> > +g_autofree char *filename = NULL;
> > int fd;
> > -const char *tmpdir;
> > -tmpdir = getenv("TMPDIR");
> > -if (!tmpdir) {
> > -tmpdir = "/var/tmp";
> > -}
> > -if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> > -return -EOVERFLOW;
> > -}
> > -fd = mkstemp(filename);
> > +
> > +fd = g_file_open_tmp("vl.XX", , NULL);
> > if (fd < 0) {
> > -return -errno;
> > +return NULL;
> > }
> > if (close(fd) != 0) {
> > unlink(filename);
> > -return -errno;
> > +return NULL;
> > }
> > -return 0;
> > -#endif
> > +return g_steal_pointer();
> > }
>
> Oh my, what a lovely mess you're messing with!
>
> The function creates a temporary *file*, not just a filename. Makes
> sense, as creating a unique filename is inherently racy. The contract
> is clear enough ("Create a uniquely-named empty temporary file"), but
> the function name is actively misleading.
Agreed that the name is misleading.
> Of course, creating a temporary file for the caller to (re)open is also
> racy. By the time the caller gets around to it, the filename could name
> anything. Return an open file file descriptor is a better idea. It's
> basically g_file_open_tmp(). Could we rework the two users of
> get_tmp_filename() accept a file descriptor?
I looked at the 2 callers, a
[PATCH v3 40/54] tests/qtest: ide-test: Open file in binary mode
From: Xuzhou Cheng By default Windows opens file in text mode, while a POSIX compliant implementation treats text files and binary files the same. The fopen() 'mode' string can include the letter 'b' to indicate binary mode shall be used. POSIX spec says the character 'b' shall have no effect, but is allowed for ISO C standard conformance. Let's add the letter 'b' which works on both POSIX and Windows. Signed-off-by: Xuzhou Cheng Signed-off-by: Bin Meng Reviewed-by: Marc-André Lureau --- (no changes since v2) Changes in v2: - Drop ahci-test.c changes that are no longer needed tests/qtest/ide-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c index 5e3e28aea2..4ea89c26c9 100644 --- a/tests/qtest/ide-test.c +++ b/tests/qtest/ide-test.c @@ -892,7 +892,7 @@ static void cdrom_pio_impl(int nblocks) /* Prepopulate the CDROM with an interesting pattern */ generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh); g_assert_cmpint(ret, ==, patt_blocks); fclose(fh); @@ -993,7 +993,7 @@ static void test_cdrom_dma(void) prdt[0].size = cpu_to_le32(len | PRDT_EOT); generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh); g_assert_cmpint(ret, ==, 16); fclose(fh); -- 2.34.1
[PATCH v3 37/54] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
From: Bin Meng
These test cases uses "blkdebug:path/to/config:path/to/image" for
testing. On Windows, absolute file paths contain the delimiter ':'
which causes the blkdebug filename parser fail to parse filenames.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v1)
tests/qtest/ahci-test.c | 21 ++---
tests/qtest/ide-test.c | 20 ++--
2 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 1d5929d8c3..66652fed04 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type, enum
AddrMode addr,
int main(int argc, char **argv)
{
-const char *arch;
+const char *arch, *base;
int ret;
int fd;
int c;
@@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
return 0;
}
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create a temporary image */
-fd = g_file_open_tmp("qtest.XX", _path, NULL);
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
+fd = g_mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
imgfmt = "qcow2";
@@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
+fd = g_mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 25302be6dc..5e3e28aea2 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -1011,16 +1011,32 @@ static void test_cdrom_dma(void)
int main(int argc, char **argv)
{
+const char *base;
int fd;
int ret;
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create temporary blkdebug instructions */
-fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
+fd = g_mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
/* Create a temporary raw image */
-fd = g_file_open_tmp("qtest.XX", _path, NULL);
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
+fd = g_mkstemp(tmp_path);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert(ret == 0);
--
2.34.1
[PATCH v3 25/54] block/vvfat: Unify the mkdir() call
From: Bin Meng
There is a difference in the mkdir() call for win32 and non-win32
platforms, and currently is handled in the codes with #ifdefs.
glib provides a portable g_mkdir() API and we can use it to unify
the codes without #ifdefs.
Signed-off-by: Bin Meng
Reviewed-by: Marc-André Lureau
---
(no changes since v2)
Changes in v2:
- Change to use g_mkdir()
block/vvfat.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..723beef025 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include
+#include
#include "qapi/error.h"
#include "block/block_int.h"
#include "block/qdict.h"
@@ -2726,13 +2727,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
mapping_t* mapping;
int j, parent_path_len;
-#ifdef __MINGW32__
-if (mkdir(commit->path))
+if (g_mkdir(commit->path, 0755)) {
return -5;
-#else
-if (mkdir(commit->path, 0755))
-return -5;
-#endif
+}
mapping = insert_mapping(s, commit->param.mkdir.cluster,
commit->param.mkdir.cluster + 1);
--
2.34.1
[PATCH v3 19/54] tests/qtest: virtio-blk-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
---
Changes in v3:
- Split to a separate patch
tests/qtest/virtio-blk-test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c
index dc5eed31c8..19c01f808b 100644
--- a/tests/qtest/virtio-blk-test.c
+++ b/tests/qtest/virtio-blk-test.c
@@ -49,10 +49,10 @@ static void drive_destroy(void *path)
static char *drive_create(void)
{
int fd, ret;
-char *t_path = g_strdup("/tmp/qtest.XX");
+char *t_path;
/* Create a temporary raw image */
-fd = mkstemp(t_path);
+fd = g_file_open_tmp("qtest.XX", _path, NULL);
g_assert_cmpint(fd, >=, 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert_cmpint(ret, ==, 0);
--
2.34.1
[PATCH v3 13/54] tests/qtest: ide-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
---
Changes in v3:
- Split to a separate patch
tests/qtest/ide-test.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 5bcb75a7e5..25302be6dc 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -121,8 +121,8 @@ enum {
static QPCIBus *pcibus = NULL;
static QGuestAllocator guest_malloc;
-static char tmp_path[] = "/tmp/qtest.XX";
-static char debug_path[] = "/tmp/qtest-blkdebug.XX";
+static char *tmp_path;
+static char *debug_path;
static QTestState *ide_test_start(const char *cmdline_fmt, ...)
{
@@ -1015,12 +1015,12 @@ int main(int argc, char **argv)
int ret;
/* Create temporary blkdebug instructions */
-fd = mkstemp(debug_path);
+fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
g_assert(fd >= 0);
close(fd);
/* Create a temporary raw image */
-fd = mkstemp(tmp_path);
+fd = g_file_open_tmp("qtest.XX", _path, NULL);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert(ret == 0);
@@ -1049,7 +1049,9 @@ int main(int argc, char **argv)
/* Cleanup */
unlink(tmp_path);
+g_free(tmp_path);
unlink(debug_path);
+g_free(debug_path);
return ret;
}
--
2.34.1
[PATCH v3 05/54] tests/qtest: ahci-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
---
Changes in v3:
- Split to a separate patch
- Ensure g_autofree variable is initialized
tests/qtest/ahci-test.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index f1e510b0ac..1d5929d8c3 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -44,9 +44,9 @@
#define TEST_IMAGE_SIZE_MB_SMALL 64
/*** Globals ***/
-static char tmp_path[] = "/tmp/qtest.XX";
-static char debug_path[] = "/tmp/qtest-blkdebug.XX";
-static char mig_socket[] = "/tmp/qtest-migration.XX";
+static char *tmp_path;
+static char *debug_path;
+static char *mig_socket;
static bool ahci_pedantic;
static const char *imgfmt;
static unsigned test_image_size_mb;
@@ -1437,10 +1437,10 @@ static void test_ncq_simple(void)
static int prepare_iso(size_t size, unsigned char **buf, char **name)
{
-char cdrom_path[] = "/tmp/qtest.iso.XX";
+g_autofree char *cdrom_path = NULL;
unsigned char *patt;
ssize_t ret;
-int fd = mkstemp(cdrom_path);
+int fd = g_file_open_tmp("qtest.iso.XX", _path, NULL);
g_assert(fd != -1);
g_assert(buf);
@@ -1872,7 +1872,7 @@ int main(int argc, char **argv)
}
/* Create a temporary image */
-fd = mkstemp(tmp_path);
+fd = g_file_open_tmp("qtest.XX", _path, NULL);
g_assert(fd >= 0);
if (have_qemu_img()) {
imgfmt = "qcow2";
@@ -1889,12 +1889,12 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-fd = mkstemp(debug_path);
+fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
g_assert(fd >= 0);
close(fd);
/* Reserve a hollow file to use as a socket for migration tests */
-fd = mkstemp(mig_socket);
+fd = g_file_open_tmp("qtest-migration.XX", _socket, NULL);
g_assert(fd >= 0);
close(fd);
@@ -1947,8 +1947,11 @@ int main(int argc, char **argv)
/* Cleanup */
unlink(tmp_path);
+g_free(tmp_path);
unlink(debug_path);
+g_free(debug_path);
unlink(mig_socket);
+g_free(mig_socket);
return ret;
}
--
2.34.1
[PATCH v3 00/54] tests/qtest: Enable running qtest on Windows
In preparation to adding virtio-9p support on Windows, this series
enables running qtest on Windows, so that we can run the virtio-9p
tests on Windows to make sure it does not break accidently.
Changes in v3:
- Remove unnecessary "error = NULL" statements
- Split patch "tests: Avoid using hardcoded /tmp in test cases" to separate
patches
- Ensure g_autofree variable is initialized
- Use g_steal_pointer() in create_test_img()
- Add another case "boot-serial-test" to justify the change
- Add a usleep(1) in the busy wait loop
- Drop the host test
- Drop patch: "tests: Change to use g_mkdir()"
- Drop patch: "block: Unify the get_tmp_filename() implementation",
and send it as a separate patch
- Drop patch: "chardev/char-file: Add FILE_SHARE_WRITE when openning the file
for win32"
Changes in v2:
- new patch: "tests/qtest: i440fx-test: Rewrite create_blob_file() to be
portable"
- Use g_autofree to declare the variable
- Change to use g_mkdir()
- Change to use g_mkdir()
- Change to use g_mkdir()
- Change to skip only part of the virtio-net-test cases that require
socketpair() intead of disabling all of them
- Introduce a new variable qtests_filter and add that to the
qtests_ARCH variables
- Add a comment in the code to explain why test_qmp_oob test case
is skipped on win32
- Replace signal by the semaphore on posix too
- Use __declspec(selectany) for the common weak symbol on Windows
- Introduce qemu_send_full() and use it
- Move the enabling of building qtests on Windows to a separate
patch to keep bisectablity
- Call socket_init() unconditionally
- Add a missing CloseHandle() call
- Change the place that sets IO redirection in the command line
- Drop ahci-test.c changes that are no longer needed
- Update commit message to include the use case why we should set
FILE_SHARE_WRITE when opening the file for win32
- Change to a busy wait after migration is canceled
- new patch: "io/channel-watch: Drop the unnecessary cast"
- Change the timeout limit to 90 minutes
- new patch: Display meson test logs in the Windows CI
- new patch: "tests/qtest: Enable qtest build on Windows"
- Minor wording changes
- Drop patches that were already applied in the mainline
- Drop patch: "qga/commands-posix-ssh: Use g_mkdir_with_parents()"
- Drop patch: "tests: Skip iotests and qtest when '--without-default-devices'"
- Drop patch: "tests/qtest: Fix ERROR_SHARING_VIOLATION for win32"
Bin Meng (47):
tests/qtest: i440fx-test: Rewrite create_blob_file() to be portable
semihosting/arm-compat-semi: Avoid using hardcoded /tmp
tcg: Avoid using hardcoded /tmp
util/qemu-sockets: Use g_get_tmp_dir() to get the directory for
temporary files
tests/qtest: ahci-test: Avoid using hardcoded /tmp
tests/qtest: aspeed_smc-test: Avoid using hardcoded /tmp
tests/qtest: boot-serial-test: Avoid using hardcoded /tmp
tests/qtest: cxl-test: Avoid using hardcoded /tmp
tests/qtest: fdc-test: Avoid using hardcoded /tmp
tests/qtest: generic_fuzz: Avoid using hardcoded /tmp
tests/qtest: virtio_blk_fuzz: Avoid using hardcoded /tmp
tests/qtest: hd-geo-test: Avoid using hardcoded /tmp
tests/qtest: ide-test: Avoid using hardcoded /tmp
tests/qtest: migration-test: Avoid using hardcoded /tmp
tests/qtest: pflash-cfi02-test: Avoid using hardcoded /tmp
tests/qtest: qmp-test: Avoid using hardcoded /tmp
tests/qtest: vhost-user-blk-test: Avoid using hardcoded /tmp
tests/qtest: vhost-user-test: Avoid using hardcoded /tmp
tests/qtest: virtio-blk-test: Avoid using hardcoded /tmp
tests/qtest: virtio-scsi-test: Avoid using hardcoded /tmp
tests/qtest: libqtest: Avoid using hardcoded /tmp
tests/unit: test-image-locking: Avoid using hardcoded /tmp
tests/unit: test-qga: Avoid using hardcoded /tmp
tests: vhost-user-bridge: Avoid using hardcoded /tmp
block/vvfat: Unify the mkdir() call
fsdev/virtfs-proxy-helper: Use g_mkdir()
hw/usb: dev-mtp: Use g_mkdir()
tests/qtest: Skip running virtio-net-test cases that require
socketpair() for win32
tests/qtest: Build test-filter-{mirror,redirector} cases for posix
only
tests/qtest: qmp-test: Skip running test_qmp_oob for win32
tests/qtest: libqtest: Exclude the *_fds APIs for win32
tests/qtest: libqtest: Install signal handler via signal()
tests/qtest: Support libqtest to build and run on Windows
tests/qtest: {ahci,ide}-test: Use relative path for temporary files
for win32
tests/qtest: bios-tables-test: Adapt the case for win32
tests/qtest: migration-test: Disable IO redirection for win32
tests/qtest: microbit-test: Fix socket access for win32
tests/qtest: libqtest: Replace the call to close a socket with
closesocket()
tests/qtest: libqtest: Correct the timeout unit of blocking receive
calls for win32
io/channel-watch: Drop a superfluous '#ifdef WIN32'
io/channel-watch: Drop the unnecessary cast
io/channe
[PATCH v3 09/54] tests/qtest: fdc-test: Avoid using hardcoded /tmp
From: Bin Meng
This case was written to use hardcoded /tmp directory for temporary
files. Update to use g_file_open_tmp() for a portable implementation.
Signed-off-by: Bin Meng
---
Changes in v3:
- Split to a separate patch
tests/qtest/fdc-test.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 52ade90a7d..1f9b99ad6d 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -68,7 +68,7 @@ enum {
DSKCHG = 0x80,
};
-static char test_image[] = "/tmp/qtest.XX";
+static char *test_image;
#define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==, (mask))
#define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==, 0)
@@ -608,7 +608,7 @@ int main(int argc, char **argv)
int ret;
/* Create a temporary raw image */
-fd = mkstemp(test_image);
+fd = g_file_open_tmp("qtest.XX", _image, NULL);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert(ret == 0);
@@ -640,6 +640,7 @@ int main(int argc, char **argv)
/* Cleanup */
qtest_end();
unlink(test_image);
+g_free(test_image);
return ret;
}
--
2.34.1
[PATCH v3] block: Refactor get_tmp_filename()
From: Bin Meng
At present there are two callers of get_tmp_filename() and they are
inconsistent.
One does:
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char *tmp_filename = g_malloc0(PATH_MAX + 1);
...
ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
while the other does:
s->qcow_filename = g_malloc(PATH_MAX);
ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
As we can see different 'size' arguments are passed. There are also
platform specific implementations inside the function, and this use
of snprintf is really undesirable.
Refactor this routine by changing its signature to:
int get_tmp_filename(char **filename)
and use g_file_open_tmp() for a consistent implementation.
Signed-off-by: Bin Meng
---
Changes in v3:
- Do not use errno directly, instead still let get_tmp_filename() return
a negative number to indicate error
Changes in v2:
- Use g_autofree and g_steal_pointer
include/block/block_int-common.h | 2 +-
block.c | 36 ++--
block/vvfat.c| 3 +--
3 files changed, 13 insertions(+), 28 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..eb30193198 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild *child)
}
int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
-int get_tmp_filename(char *filename, int size);
+int get_tmp_filename(char **filename);
void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
QDict *options);
diff --git a/block.c b/block.c
index bc85f46eed..55ad257241 100644
--- a/block.c
+++ b/block.c
@@ -861,37 +861,24 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
/*
* Create a uniquely-named empty temporary file.
* Return 0 upon success, otherwise a negative errno value.
+ * The caller function is responsible for freeing *filename.
*/
-int get_tmp_filename(char *filename, int size)
+int get_tmp_filename(char **filename)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
+g_autofree char *name = NULL;
int fd;
-const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
-if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
-return -EOVERFLOW;
-}
-fd = mkstemp(filename);
+
+fd = g_file_open_tmp("vl.XX", , NULL);
if (fd < 0) {
-return -errno;
+return -ENOENT;
}
if (close(fd) != 0) {
-unlink(filename);
+unlink(name);
return -errno;
}
+
+*filename = g_steal_pointer();
return 0;
-#endif
}
/*
@@ -3717,8 +3704,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
QDict *snapshot_options,
Error **errp)
{
-/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
-char *tmp_filename = g_malloc0(PATH_MAX + 1);
+char *tmp_filename = NULL;
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
@@ -3737,7 +3723,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
}
/* Create the temporary image */
-ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
+ret = get_tmp_filename(_filename);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not get temporary filename");
goto out;
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..43f42fd1ea 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3146,8 +3146,7 @@ static int enable_write_target(BlockDriverState *bs,
Error **errp)
array_init(&(s->commits), sizeof(commit_t));
-s->qcow_filename = g_malloc(PATH_MAX);
-ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
+ret = get_tmp_filename(>qcow_filename);
if (ret < 0) {
error_setg_errno(errp, -ret, "can't create temporary file");
goto err;
--
2.34.1
[PATCH v2] block: Refactor get_tmp_filename()
From: Bin Meng
At present there are two callers of get_tmp_filename() and they are
inconsistent.
One does:
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char *tmp_filename = g_malloc0(PATH_MAX + 1);
...
ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
while the other does:
s->qcow_filename = g_malloc(PATH_MAX);
ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
As we can see different 'size' arguments are passed. There are also
platform specific implementations inside the function, and this use
of snprintf is really undesirable.
Refactor this routine by changing its signature to:
char *get_tmp_filename(void)
and use g_file_open_tmp() for a consistent implementation.
Signed-off-by: Bin Meng
---
Changes in v2:
- Use g_autofree and g_steal_pointer
include/block/block_int-common.h | 2 +-
block.c | 42 ++--
block/vvfat.c| 8 +++---
3 files changed, 18 insertions(+), 34 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..ea69a9349c 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild *child)
}
int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
-int get_tmp_filename(char *filename, int size);
+char *get_tmp_filename(void);
void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
QDict *options);
diff --git a/block.c b/block.c
index bc85f46eed..4e7a795566 100644
--- a/block.c
+++ b/block.c
@@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
/*
* Create a uniquely-named empty temporary file.
- * Return 0 upon success, otherwise a negative errno value.
+ * Return the actual name used upon success, otherwise NULL.
+ * The called function is responsible for freeing it.
*/
-int get_tmp_filename(char *filename, int size)
+char *get_tmp_filename(void)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
+g_autofree char *filename = NULL;
int fd;
-const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
-if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
-return -EOVERFLOW;
-}
-fd = mkstemp(filename);
+
+fd = g_file_open_tmp("vl.XX", , NULL);
if (fd < 0) {
-return -errno;
+return NULL;
}
if (close(fd) != 0) {
unlink(filename);
-return -errno;
+return NULL;
}
-return 0;
-#endif
+return g_steal_pointer();
}
/*
@@ -3717,8 +3702,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
QDict *snapshot_options,
Error **errp)
{
-/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
-char *tmp_filename = g_malloc0(PATH_MAX + 1);
+char *tmp_filename = NULL;
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
@@ -3737,9 +3721,9 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
}
/* Create the temporary image */
-ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not get temporary filename");
+tmp_filename = get_tmp_filename();
+if (!tmp_filename) {
+error_setg_errno(errp, errno, "Could not get temporary filename");
goto out;
}
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..135fafb166 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3146,10 +3146,10 @@ static int enable_write_target(BlockDriverState *bs,
Error **errp)
array_init(&(s->commits), sizeof(commit_t));
-s->qcow_filename = g_malloc(PATH_MAX);
-ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "can't create temporary file");
+s->qcow_filename = get_tmp_filename();
+if (!s->qcow_filename) {
+error_setg_errno(errp, errno, "can't create temporary file");
+ret = -errno;
goto err;
}
--
2.34.1
Re: [PATCH v2 03/39] block: Unify the get_tmp_filename() implementation
On Fri, Sep 23, 2022 at 3:39 AM Marc-André Lureau wrote: > > Hi > > On Tue, Sep 20, 2022 at 2:17 PM Bin Meng wrote: >> >> From: Bin Meng >> >> At present get_tmp_filename() has platform specific implementations >> to get the directory to use for temporary files. Switch over to use >> g_get_tmp_dir() which works on all supported platforms. >> > > As discussed in v1, there are other things to do while touching this code. > And since it is optional for the series goal, please send this as a different > patch/series. > Okay, a separate patch has just been sent. Regards, Bin
[PATCH] block: Refactor get_tmp_filename()
From: Bin Meng
At present there are two callers of get_tmp_filename() and they are
inconsistent.
One does:
/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
char *tmp_filename = g_malloc0(PATH_MAX + 1);
...
ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
while the other does:
s->qcow_filename = g_malloc(PATH_MAX);
ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
As we can see different 'size' arguments are passed. There are also
platform specific implementations inside the function, and this use
of snprintf is really undesirable.
Refactor this routine by changing its signature to:
char *get_tmp_filename(void)
and use g_file_open_tmp() for a consistent implementation.
Signed-off-by: Bin Meng
---
include/block/block_int-common.h | 2 +-
block.c | 42 ++--
block/vvfat.c| 8 +++---
3 files changed, 18 insertions(+), 34 deletions(-)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..ea69a9349c 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild *child)
}
int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
-int get_tmp_filename(char *filename, int size);
+char *get_tmp_filename(void);
void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
QDict *options);
diff --git a/block.c b/block.c
index bc85f46eed..0f6c460199 100644
--- a/block.c
+++ b/block.c
@@ -860,38 +860,23 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
/*
* Create a uniquely-named empty temporary file.
- * Return 0 upon success, otherwise a negative errno value.
+ * Return the actual name used upon success, otherwise NULL.
+ * The called function is responsible for freeing it.
*/
-int get_tmp_filename(char *filename, int size)
+char *get_tmp_filename(void)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
+char *filename;
int fd;
-const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
-if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
-return -EOVERFLOW;
-}
-fd = mkstemp(filename);
+
+fd = g_file_open_tmp("vl.XX", , NULL);
if (fd < 0) {
-return -errno;
+return NULL;
}
if (close(fd) != 0) {
unlink(filename);
-return -errno;
+return NULL;
}
-return 0;
-#endif
+return filename;
}
/*
@@ -3717,8 +3702,7 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
QDict *snapshot_options,
Error **errp)
{
-/* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
-char *tmp_filename = g_malloc0(PATH_MAX + 1);
+char *tmp_filename = NULL;
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
@@ -3737,9 +3721,9 @@ static BlockDriverState
*bdrv_append_temp_snapshot(BlockDriverState *bs,
}
/* Create the temporary image */
-ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not get temporary filename");
+tmp_filename = get_tmp_filename();
+if (!tmp_filename) {
+error_setg_errno(errp, errno, "Could not get temporary filename");
goto out;
}
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..135fafb166 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3146,10 +3146,10 @@ static int enable_write_target(BlockDriverState *bs,
Error **errp)
array_init(&(s->commits), sizeof(commit_t));
-s->qcow_filename = g_malloc(PATH_MAX);
-ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
-if (ret < 0) {
-error_setg_errno(errp, -ret, "can't create temporary file");
+s->qcow_filename = get_tmp_filename();
+if (!s->qcow_filename) {
+error_setg_errno(errp, errno, "can't create temporary file");
+ret = -errno;
goto err;
}
--
2.34.1
Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
Hi,
On Wed, Sep 21, 2022 at 8:10 PM Meng, Bin wrote:
>
> -Original Message-
> From: Philippe Mathieu-Daudé On Behalf Of
> Philippe Mathieu-Daudé
> Sent: Sunday, September 18, 2022 5:32 AM
> To: Bin Meng ; qemu-de...@nongnu.org
> Cc: Meng, Bin ; Hanna Reitz ;
> Kevin Wolf ; Peter Lieven ;
> qemu-block@nongnu.org
> Subject: Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
>
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On 8/9/22 15:28, Bin Meng wrote:
> > From: Bin Meng
> >
> > libnfs.h declares nfs_fstat() as the following for win32:
> >
> >int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
> > struct __stat64 *st);
> >
> > The 'st' parameter should be of type 'struct __stat64'. The codes
> > happen to build successfully for 64-bit Windows, but it does not build
> > for 32-bit Windows.
> >
> > Fixes: 6542aa9c75bc ("block: add native support for NFS")
> > Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for
> > read-only files")
> > Signed-off-by: Bin Meng
> > ---
> >
> > block/nfs.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/block/nfs.c b/block/nfs.c index 444c40b458..d5d67937dd
> > 100644
> > --- a/block/nfs.c
> > +++ b/block/nfs.c
> > @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client,
> > BlockdevOptionsNfs *opts,
> > int flags, int open_flags, Error **errp)
> > {
> > int64_t ret = -EINVAL;
> > +#ifdef _WIN32
> > +struct __stat64 st;
> > +#else
> > struct stat st;
> > +#endif
> > char *file = NULL, *strp = NULL;
> >
> > qemu_mutex_init(>mutex); @@ -781,7 +785,11 @@ static int
> > nfs_reopen_prepare(BDRVReopenState *state,
> > BlockReopenQueue *queue, Error **errp)
> > {
> > NFSClient *client = state->bs->opaque;
> > +#ifdef _WIN32
> > +struct __stat64 st;
> > +#else
> > struct stat st;
> > +#endif
> > int ret = 0;
> >
> > if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs))
> > {
>
> What about the field in struct NFSRPC?
>
> NFSRPC::struct stat is used in nfs_get_allocated_file_size_cb() and
> nfs_get_allocated_file_size() that are not built on win32, so there is no
> problem.
>
Any further comments?
Regards,
Bin
Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
Hi Stefan,
On Wed, Sep 21, 2022 at 8:24 PM Thomas Huth wrote:
>
> On 21/09/2022 14.18, Bin Meng wrote:
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 9:28 PM Bin Meng wrote:
> >>
> >> At present packaging the required DLLs of QEMU executables is a
> >> manual process, and error prone.
> >>
> >> Improve scripts/nsis.py by adding a logic to automatically package
> >> required DLLs of QEMU executables.
> >>
> >> 'make installer' is tested in the cross-build on Linux in CI, but
> >> not in the Windows native build. Update CI to test the installer
> >> generation on Windows too.
> >>
> >> During testing a 32-bit build issue was exposed in block/nfs.c and
> >> the fix is included in this series.
> >>
> >>
> >> Bin Meng (7):
> >>scripts/nsis.py: Drop the unnecessary path separator
> >>scripts/nsis.py: Fix destination directory name when invoked on
> >> Windows
> >>scripts/nsis.py: Automatically package required DLLs of QEMU
> >> executables
> >>.gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
> >>block/nfs: Fix 32-bit Windows build
> >>.gitlab-ci.d/windows.yml: Unify the prerequisite packages
> >>.gitlab-ci.d/windows.yml: Test 'make installer' in the CI
> >>
> >> meson.build | 1 +
> >> block/nfs.c | 8 ++
> >> .gitlab-ci.d/windows.yml | 40 ---
> >> scripts/nsis.py | 60 +---
> >> 4 files changed, 89 insertions(+), 20 deletions(-)
> >>
> >
> > I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
> > sed processing in the 64-bit build")
> >
> > What about other patches?
>
> I hope that Stefan Weil (our W32 maintainer) could have a look at these
> first...
Would you please comment this series? Thanks!
Regards,
Bin
Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
Hi,
On Thu, Sep 8, 2022 at 9:28 PM Bin Meng wrote:
>
> At present packaging the required DLLs of QEMU executables is a
> manual process, and error prone.
>
> Improve scripts/nsis.py by adding a logic to automatically package
> required DLLs of QEMU executables.
>
> 'make installer' is tested in the cross-build on Linux in CI, but
> not in the Windows native build. Update CI to test the installer
> generation on Windows too.
>
> During testing a 32-bit build issue was exposed in block/nfs.c and
> the fix is included in this series.
>
>
> Bin Meng (7):
> scripts/nsis.py: Drop the unnecessary path separator
> scripts/nsis.py: Fix destination directory name when invoked on
> Windows
> scripts/nsis.py: Automatically package required DLLs of QEMU
> executables
> .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
> block/nfs: Fix 32-bit Windows build
> .gitlab-ci.d/windows.yml: Unify the prerequisite packages
> .gitlab-ci.d/windows.yml: Test 'make installer' in the CI
>
> meson.build | 1 +
> block/nfs.c | 8 ++
> .gitlab-ci.d/windows.yml | 40 ---
> scripts/nsis.py | 60 +---
> 4 files changed, 89 insertions(+), 20 deletions(-)
>
I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
sed processing in the 64-bit build")
What about other patches?
Regards,
Bin
[PATCH v2 23/39] tests/qtest: ide-test: Open file in binary mode
From: Xuzhou Cheng By default Windows opens file in text mode, while a POSIX compliant implementation treats text files and binary files the same. The fopen() 'mode' string can include the letter 'b' to indicate binary mode shall be used. POSIX spec says the character 'b' shall have no effect, but is allowed for ISO C standard conformance. Let's add the letter 'b' which works on both POSIX and Windows. Signed-off-by: Xuzhou Cheng Signed-off-by: Bin Meng Reviewed-by: Marc-André Lureau --- Changes in v2: - Drop ahci-test.c changes that are no longer needed tests/qtest/ide-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c index 5e3e28aea2..4ea89c26c9 100644 --- a/tests/qtest/ide-test.c +++ b/tests/qtest/ide-test.c @@ -892,7 +892,7 @@ static void cdrom_pio_impl(int nblocks) /* Prepopulate the CDROM with an interesting pattern */ generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh); g_assert_cmpint(ret, ==, patt_blocks); fclose(fh); @@ -993,7 +993,7 @@ static void test_cdrom_dma(void) prdt[0].size = cpu_to_le32(len | PRDT_EOT); generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh); g_assert_cmpint(ret, ==, 16); fclose(fh); -- 2.34.1
[PATCH v2 20/39] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
From: Bin Meng
These test cases uses "blkdebug:path/to/config:path/to/image" for
testing. On Windows, absolute file paths contain the delimiter ':'
which causes the blkdebug filename parser fail to parse filenames.
Signed-off-by: Bin Meng
---
(no changes since v1)
tests/qtest/ahci-test.c | 21 ++---
tests/qtest/ide-test.c | 20 ++--
2 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 00524f14c6..c57576b08c 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type, enum
AddrMode addr,
int main(int argc, char **argv)
{
-const char *arch;
+const char *arch, *base;
int ret;
int fd;
int c;
@@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
return 0;
}
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create a temporary image */
-fd = g_file_open_tmp("qtest.XX", _path, NULL);
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
+fd = g_mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
imgfmt = "qcow2";
@@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
+fd = g_mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 25302be6dc..5e3e28aea2 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -1011,16 +1011,32 @@ static void test_cdrom_dma(void)
int main(int argc, char **argv)
{
+const char *base;
int fd;
int ret;
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create temporary blkdebug instructions */
-fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
+fd = g_mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
/* Create a temporary raw image */
-fd = g_file_open_tmp("qtest.XX", _path, NULL);
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
+fd = g_mkstemp(tmp_path);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
g_assert(ret == 0);
--
2.34.1
[PATCH v2 07/39] tests: Avoid using hardcoded /tmp in test cases
From: Bin Meng
Lots of test cases were written to use hardcoded /tmp directory for
temporary files. To avoid this, we update them to use g_dir_make_tmp()
or g_file_open_tmp() when appropriate.
Signed-off-by: Bin Meng
---
Changes in v2:
- Use g_dir_make_tmp(), g_file_open_tmp() when appropriate
tests/qtest/fuzz/generic_fuzz_configs.h | 4 ++--
tests/qtest/ahci-test.c | 19 +++
tests/qtest/aspeed_smc-test.c | 5 ++---
tests/qtest/boot-serial-test.c | 9 +
tests/qtest/cxl-test.c | 15 ++-
tests/qtest/fdc-test.c | 5 +++--
tests/qtest/fuzz/virtio_blk_fuzz.c | 4 ++--
tests/qtest/hd-geo-test.c | 24 +++-
tests/qtest/ide-test.c | 10 ++
tests/qtest/libqtest.c | 12
tests/qtest/migration-test.c| 7 ---
tests/qtest/pflash-cfi02-test.c | 8 +---
tests/qtest/qmp-test.c | 6 --
tests/qtest/vhost-user-blk-test.c | 3 ++-
tests/qtest/vhost-user-test.c | 8
tests/qtest/virtio-blk-test.c | 4 ++--
tests/qtest/virtio-scsi-test.c | 4 ++--
tests/unit/test-image-locking.c | 8
tests/unit/test-qga.c | 2 +-
tests/vhost-user-bridge.c | 3 +--
20 files changed, 85 insertions(+), 75 deletions(-)
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 0775e6702b..a825b78c14 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -20,8 +20,8 @@ typedef struct generic_fuzz_config {
} generic_fuzz_config;
static inline gchar *generic_fuzzer_virtio_9p_args(void){
-char tmpdir[] = "/tmp/qemu-fuzz.XX";
-g_assert_nonnull(g_mkdtemp(tmpdir));
+g_autofree char *tmpdir = g_dir_make_tmp("qemu-fuzz.XX", NULL);
+g_assert_nonnull(tmpdir);
return g_strdup_printf("-machine q35 -nodefaults "
"-device virtio-9p,fsdev=hshare,mount_tag=hshare "
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index f1e510b0ac..00524f14c6 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -44,9 +44,9 @@
#define TEST_IMAGE_SIZE_MB_SMALL 64
/*** Globals ***/
-static char tmp_path[] = "/tmp/qtest.XX";
-static char debug_path[] = "/tmp/qtest-blkdebug.XX";
-static char mig_socket[] = "/tmp/qtest-migration.XX";
+static char *tmp_path;
+static char *debug_path;
+static char *mig_socket;
static bool ahci_pedantic;
static const char *imgfmt;
static unsigned test_image_size_mb;
@@ -1437,10 +1437,10 @@ static void test_ncq_simple(void)
static int prepare_iso(size_t size, unsigned char **buf, char **name)
{
-char cdrom_path[] = "/tmp/qtest.iso.XX";
+g_autofree char *cdrom_path;
unsigned char *patt;
ssize_t ret;
-int fd = mkstemp(cdrom_path);
+int fd = g_file_open_tmp("qtest.iso.XX", _path, NULL);
g_assert(fd != -1);
g_assert(buf);
@@ -1872,7 +1872,7 @@ int main(int argc, char **argv)
}
/* Create a temporary image */
-fd = mkstemp(tmp_path);
+fd = g_file_open_tmp("qtest.XX", _path, NULL);
g_assert(fd >= 0);
if (have_qemu_img()) {
imgfmt = "qcow2";
@@ -1889,12 +1889,12 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-fd = mkstemp(debug_path);
+fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
g_assert(fd >= 0);
close(fd);
/* Reserve a hollow file to use as a socket for migration tests */
-fd = mkstemp(mig_socket);
+fd = g_file_open_tmp("qtest-migration.XX", _socket, NULL);
g_assert(fd >= 0);
close(fd);
@@ -1947,8 +1947,11 @@ int main(int argc, char **argv)
/* Cleanup */
unlink(tmp_path);
+g_free(tmp_path);
unlink(debug_path);
+g_free(debug_path);
unlink(mig_socket);
+g_free(mig_socket);
return ret;
}
diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index 05ce941566..5e16b5c9a5 100644
--- a/tests/qtest/aspeed_smc-test.c
+++ b/tests/qtest/aspeed_smc-test.c
@@ -608,16 +608,15 @@ static void test_write_block_protect_bottom_bit(void)
flash_reset();
}
-static char tmp_path[] = "/tmp/qtest.m25p80.XX";
-
int main(int argc, char **argv)
{
+g_autofree char *tmp_path;
int ret;
int fd;
g_test_init(, , NULL);
-fd = mkstemp(tmp_path);
+fd = g_file_open_tmp("qtest.m25p80.XX", _path, NULL);
g_assert(fd >= 0);
ret = ftruncate(fd, FLASH_SIZE);
g_assert(ret == 0);
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 2f99d71cab..ce6e3
[PATCH v2 03/39] block: Unify the get_tmp_filename() implementation
From: Bin Meng
At present get_tmp_filename() has platform specific implementations
to get the directory to use for temporary files. Switch over to use
g_get_tmp_dir() which works on all supported platforms.
Signed-off-by: Bin Meng
---
(no changes since v1)
block.c | 16 ++--
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index bc85f46eed..d06df47f72 100644
--- a/block.c
+++ b/block.c
@@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
*/
int get_tmp_filename(char *filename, int size)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
int fd;
const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
+tmpdir = g_get_tmp_dir();
+
if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
return -EOVERFLOW;
}
@@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
return -errno;
}
return 0;
-#endif
}
/*
--
2.34.1
[PATCH v2 00/39] tests/qtest: Enable running qtest on Windows
In preparation to adding virtio-9p support on Windows, this series
enables running qtest on Windows, so that we can run the virtio-9p
tests on Windows to make sure it does not break accidently.
Changes in v2:
- new patch: "tests: Change to use g_mkdir()"
- new patch: "tests/qtest: i440fx-test: Rewrite create_blob_file() to be
portable"
- Use g_autofree to declare the variable
- Use g_dir_make_tmp(), g_file_open_tmp() when appropriate
- Change to use g_mkdir()
- Change to use g_mkdir()
- Change to use g_mkdir()
- Change to skip only part of the virtio-net-test cases that require
socketpair() intead of disabling all of them
- Introduce a new variable qtests_filter and add that to the
qtests_ARCH variables
- Add a comment in the code to explain why test_qmp_oob test case
is skipped on win32
- Replace signal by the semaphore on posix too
- Use __declspec(selectany) for the common weak symbol on Windows
- Introduce qemu_send_full() and use it
- Move the enabling of building qtests on Windows to a separate
patch to keep bisectablity
- Call socket_init() unconditionally
- Add a missing CloseHandle() call
- Change the place that sets IO redirection in the command line
- Drop ahci-test.c changes that are no longer needed
- Update commit message to include the use case why we should set
FILE_SHARE_WRITE when openning the file for win32
- Change to a busy wait after migration is canceled
- new patch: "hw/pci-host: pnv_phb{3,4}: Fix heap out-of-bound access failure"
- new patch: "io/channel-watch: Drop the unnecessary cast"
- Change the timeout limit to 90 minutes
- new patch: Display meson test logs in the Windows CI
- new patch: "tests/qtest: Enable qtest build on Windows"
- Minor wording changes
- Drop patches that were already applied in the mainline
- Drop patch: "qga/commands-posix-ssh: Use g_mkdir_with_parents()"
- Drop patch: "tests: Skip iotests and qtest when '--without-default-devices'"
- Drop patch: "tests/qtest: Fix ERROR_SHARING_VIOLATION for win32"
Bin Meng (30):
tests: Change to use g_mkdir()
tests/qtest: i440fx-test: Rewrite create_blob_file() to be portable
block: Unify the get_tmp_filename() implementation
semihosting/arm-compat-semi: Avoid using hardcoded /tmp
tcg: Avoid using hardcoded /tmp
util/qemu-sockets: Use g_get_tmp_dir() to get the directory for
temporary files
tests: Avoid using hardcoded /tmp in test cases
block/vvfat: Unify the mkdir() call
fsdev/virtfs-proxy-helper: Use g_mkdir()
hw/usb: dev-mtp: Use g_mkdir()
tests/qtest: Skip running virtio-net-test cases that require
socketpair() for win32
tests/qtest: Build test-filter-{mirror,redirector} cases for posix
only
tests/qtest: qmp-test: Skip running test_qmp_oob for win32
tests/qtest: libqtest: Exclude the *_fds APIs for win32
tests/qtest: libqtest: Install signal handler via signal()
tests/qtest: Support libqtest to build and run on Windows
tests/qtest: {ahci,ide}-test: Use relative path for temporary files
for win32
tests/qtest: bios-tables-test: Adapt the case for win32
tests/qtest: migration-test: Disable IO redirection for win32
tests/qtest: microbit-test: Fix socket access for win32
tests/qtest: libqtest: Replace the call to close a socket with
closesocket()
tests/qtest: libqtest: Correct the timeout unit of blocking receive
calls for win32
io/channel-watch: Drop a superfluous '#ifdef WIN32'
io/channel-watch: Drop the unnecessary cast
io/channel-watch: Fix socket watch on Windows
tests/qtest: migration-test: Skip running some TLS cases for win32
.gitlab-ci.d/windows.yml: Increase the timeout to 90 minutes
.gitlab-ci.d/windows.yml: Display meson test logs
tests/qtest: Enable qtest build on Windows
docs/devel: testing: Document writing portable test cases
Xuzhou Cheng (9):
accel/qtest: Implement a portable qtest accelerator
tests/qtest: libqtest: Adapt global_qtest declaration for win32
tests/qtest: Use send/recv for socket communication
tests/qtest: ide-test: Open file in binary mode
tests/qtest: virtio-net-failover: Disable migration tests for win32
chardev/char-file: Add FILE_SHARE_WRITE when openning the file for
win32
tests/qtest: migration-test: Make sure QEMU process "to" exited after
migration is canceled
hw/ppc: spapr: Use qemu_vfree() to free spapr->htab
hw/pci-host: pnv_phb{3,4}: Fix heap out-of-bound access failure
docs/devel/testing.rst | 30 +
include/hw/core/cpu.h | 1 +
include/qemu/sockets.h | 2 +
tests/qtest/fuzz/generic_fuzz_configs.h | 4 +-
tests/qtest/libqtest-single.h | 4 +
tests/qtest/libqtest.h | 8 ++
accel/dummy-cpus.c | 15 +--
block.c | 16 +--
block/vvfat.c | 9 +-
chardev/char-file.c
[PATCH v2 08/39] block/vvfat: Unify the mkdir() call
From: Bin Meng
There is a difference in the mkdir() call for win32 and non-win32
platforms, and currently is handled in the codes with #ifdefs.
glib provides a portable g_mkdir() API and we can use it to unify
the codes without #ifdefs.
Signed-off-by: Bin Meng
---
Changes in v2:
- Change to use g_mkdir()
block/vvfat.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..723beef025 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include
+#include
#include "qapi/error.h"
#include "block/block_int.h"
#include "block/qdict.h"
@@ -2726,13 +2727,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
mapping_t* mapping;
int j, parent_path_len;
-#ifdef __MINGW32__
-if (mkdir(commit->path))
+if (g_mkdir(commit->path, 0755)) {
return -5;
-#else
-if (mkdir(commit->path, 0755))
-return -5;
-#endif
+}
mapping = insert_mapping(s, commit->param.mkdir.cluster,
commit->param.mkdir.cluster + 1);
--
2.34.1
Re: [PATCH 00/11] ppc/e500: Add support for two types of flash, cleanup
Hi Bernhard, On Thu, Sep 15, 2022 at 11:25 PM Bernhard Beschow wrote: > > This series adds support for -pflash and direct SD card access to the > PPC e500 boards. The idea is to increase compatibility with "real" firmware > images where only the bare minimum of drivers is compiled in. > > The series is structured as follows: > > Patches 1-3 perform some general cleanup which paves the way for the rest of > the series. > > Patches 4-7 add -pflash handling where memory-mapped flash can be added on > user's behalf. That is, the flash memory region is only added if the -pflash > argument is supplied. Note that the cfi01 device model becomes stricter in > checking the size of the emulated flash space. > > Patches 8-11 add a new device model - the Freescale eSDHC - to the e500 > boards which was missing so far. > > User documentation is also added as the new features become available. > > Tesing done: > * `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append > "console=ttyS0 rootwait root=/dev/mtdblock0 nokaslr" -drive > if=pflash,file=rootfs.ext2,format=raw` > * `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append > "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive > -drive > id=mydrive,if=none,file=rootfs.ext2,format=raw` Thanks for the patches! Did you get a chance to test the U-Boot image to work with pflash and eSDHC? > > The load was created using latest Buildroot with `make > qemu_ppc_e500mc_defconfig` where the rootfs was configured to be of ext2 type. > In both cases it was possible to log in and explore the root file system. > Regards, Bin
Re: [PATCH 11/11] hw/ppc/e500: Add Freescale eSDHC to e500 boards
On Thu, Sep 15, 2022 at 11:30 PM Bernhard Beschow wrote:
>
> Adds missing functionality to emulated e500 SOCs which increases the
> chance of given "real" firmware images to access SD cards.
By "firmware" do you mean U-Boot?
>
> Signed-off-by: Bernhard Beschow
> ---
> docs/system/ppc/ppce500.rst | 13 +
> hw/ppc/Kconfig | 1 +
> hw/ppc/e500.c | 32
> 3 files changed, 46 insertions(+)
>
> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> index c3f55c6f3d..50b199c8f3 100644
> --- a/docs/system/ppc/ppce500.rst
> +++ b/docs/system/ppc/ppce500.rst
> @@ -19,6 +19,7 @@ The ``ppce500`` machine supports the following devices:
> * Power-off functionality via one GPIO pin
> * 1 Freescale MPC8xxx PCI host controller
> * VirtIO devices via PCI bus
> +* 1 Freescale Enhanced Secure Digital Host controller (eSDHC)
> * 1 Freescale Enhanced Triple Speed Ethernet controller (eTSEC)
>
> Hardware configuration information
> @@ -131,6 +132,18 @@ be used as follows:
>-drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
>-append "rootwait root=/dev/mtdblock0"
>
> +Alternatively, the root file system can also reside on an emulated SD card
> +whose size must again be a power of two:
> +
> +.. code-block:: bash
> +
> + $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
qemu-system-ppc{64|32}
> + -display none -serial stdio \
> + -kernel vmlinux \
> + -device sd-card,drive=mydrive \
> + -drive id=mydrive,if=none,file=/path/to/rootfs.ext2,format=raw \
> + -append "rootwait root=/dev/mmcblk0"
> +
> Running U-Boot
> --
>
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 769a1ead1c..6e31f568ba 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -129,6 +129,7 @@ config E500
> select PFLASH_CFI01
> select PLATFORM_BUS
> select PPCE500_PCI
> +select SDHCI
> select SERIAL
> select MPC_I2C
> select FDT_PPC
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 7843a4e04b..87a03fd4a9 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -48,6 +48,7 @@
> #include "hw/net/fsl_etsec/etsec.h"
> #include "hw/i2c/i2c.h"
> #include "hw/irq.h"
> +#include "hw/sd/sdhci.h"
>
> #define EPAPR_MAGIC(0x45504150)
> #define DTC_LOAD_PAD 0x180
> @@ -66,11 +67,14 @@
> #define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
> #define MPC8544_PCI_REGS_OFFSET0x8000ULL
> #define MPC8544_PCI_REGS_SIZE 0x1000ULL
> +#define MPC85XX_ESDHC_REGS_OFFSET 0x2e000ULL
> +#define MPC85XX_ESDHC_REGS_SIZE0x1000ULL
> #define MPC8544_UTIL_OFFSET0xeULL
> #define MPC8XXX_GPIO_OFFSET0x000FF000ULL
> #define MPC8544_I2C_REGS_OFFSET0x3000ULL
> #define MPC8XXX_GPIO_IRQ 47
> #define MPC8544_I2C_IRQ43
> +#define MPC85XX_ESDHC_IRQ 72
> #define RTC_REGS_OFFSET0x68
>
> #define PLATFORM_CLK_FREQ_HZ (400 * 1000 * 1000)
> @@ -203,6 +207,25 @@ static void dt_i2c_create(void *fdt, const char *soc,
> const char *mpic,
> g_free(i2c);
> }
>
> +static void dt_sdhc_create(void *fdt, const char *parent, const char *mpic)
> +{
> +hwaddr mmio = MPC85XX_ESDHC_REGS_OFFSET;
> +hwaddr size = MPC85XX_ESDHC_REGS_SIZE;
> +int irq = MPC85XX_ESDHC_IRQ;
> +char *name;
> +
> +name = g_strdup_printf("%s/sdhc@%" PRIx64, parent, mmio);
> +qemu_fdt_add_subnode(fdt, name);
> +/* qemu_fdt_setprop_cells(fdt, name, "voltage-ranges", 3300, 3300); */
Drop it if it is useless
> +qemu_fdt_setprop_cells(fdt, name, "clock-frequency", 16700);
Is this an arbitrary frequency?
> +qemu_fdt_setprop(fdt, name, "sdhci,auto-cmd12", NULL, 0);
> +qemu_fdt_setprop_phandle(fdt, name, "interrupt-parent", mpic);
> +qemu_fdt_setprop_cells(fdt, name, "bus-width", 4);
> +qemu_fdt_setprop_cells(fdt, name, "interrupts", irq, 0x2);
> +qemu_fdt_setprop_cells(fdt, name, "reg", mmio, size);
> +qemu_fdt_setprop_string(fdt, name, "compatible", "fsl,esdhc");
> +g_free(name);
> +}
>
> typedef struct PlatformDevtreeData {
> void *fdt;
> @@ -556,6 +579,8 @@ static int ppce500_load_device_tree(PPCE500MachineState
> *pms,
>
> dt_rtc_create(fdt, "i2c", "rtc");
>
> +/* sdhc */
> +dt_sdhc_create(fdt, soc, mpic);
>
> gutil = g_strdup_printf("%s/global-utilities@%llx", soc,
> MPC8544_UTIL_OFFSET);
> @@ -996,6 +1021,13 @@ void ppce500_init(MachineState *machine)
> i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
>
>
nits: use one line for the separation
> +/* eSDHC */
> +dev = qdev_new(TYPE_FSL_ESDHC);
> +s = SYS_BUS_DEVICE(dev);
> +sysbus_realize_and_unref(s, _fatal);
> +sysbus_mmio_map(s, 0, pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET);
> +sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
> +
> /* General Utility device */
>
Re: [PATCH 10/11] hw/sd/sdhci: Implement Freescale eSDHC device model
On Thu, Sep 15, 2022 at 11:30 PM Bernhard Beschow wrote:
>
> Will allow e500 boards to access SD cards using just their own devices.
>
> Signed-off-by: Bernhard Beschow
> ---
> hw/sd/sdhci.c | 147 +-
> include/hw/sd/sdhci.h | 3 +
> 2 files changed, 149 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 7a5996caad..09285ccfa1 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> sdhci_data_transfer, s);
>
> s->io_ops = _mmio_ops;
> +s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
> }
>
> void sdhci_uninitfn(SDHCIState *s)
> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>
> memory_region_init_io(>iomem, OBJECT(s), s->io_ops, s, "sdhci",
> - SDHC_REGISTERS_MAP_SIZE);
> + s->io_registers_map_size);
> }
>
> void sdhci_common_unrealize(SDHCIState *s)
> @@ -1575,6 +1576,149 @@ static const TypeInfo sdhci_bus_info = {
> .class_init = sdhci_bus_class_init,
> };
>
> +/* --- qdev Freescale eSDHC --- */
> +
> +/* Host Controller Capabilities Register 2 */
> +#define ESDHC_CAPABILITIES_10x114
> +
> +/* Control Register for DMA transfer */
> +#define ESDHC_DMA_SYSCTL0x40c
> +#define ESDHC_PERIPHERAL_CLK_SEL0x0008
> +#define ESDHC_FLUSH_ASYNC_FIFO 0x0004
> +#define ESDHC_DMA_SNOOP 0x0040
It looks the above 3 bit fields are not used?
> +
> +#define ESDHC_REGISTERS_MAP_SIZE0x410
> +
> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +uint64_t ret;
> +
> +if (size != 4) {
> +qemu_log_mask(LOG_GUEST_ERROR, "ESDHC rd_%ub @0x%02" HWADDR_PRIx
> + " wrong size\n", size, offset);
> +return 0;
> +}
> +
> +if (offset & 0x3) {
> +qemu_log_mask(LOG_GUEST_ERROR, "ESDHC rd_%ub @0x%02" HWADDR_PRIx
> + " unaligned\n", size, offset);
> +return 0;
> +}
> +
> +switch (offset) {
> +case SDHC_SYSAD:
> +case SDHC_BLKSIZE:
> +case SDHC_ARGUMENT:
> +case SDHC_TRNMOD:
> +case SDHC_RSPREG0:
> +case SDHC_RSPREG1:
> +case SDHC_RSPREG2:
> +case SDHC_RSPREG3:
> +case SDHC_BDATA:
> +case SDHC_PRNSTS:
> +case SDHC_HOSTCTL:
> +case SDHC_CLKCON:
> +case SDHC_NORINTSTS:
> +case SDHC_NORINTSTSEN:
> +case SDHC_NORINTSIGEN:
> +case SDHC_ACMD12ERRSTS:
> +case SDHC_CAPAB:
> +case SDHC_SLOT_INT_STATUS:
> +ret = sdhci_read(opaque, offset, size);
> +break;
> +
> +case ESDHC_DMA_SYSCTL:
> +case 0x44:
Can we define a macro for this offset?
> +ret = 0;
> +qemu_log_mask(LOG_UNIMP, "ESDHC rd_%ub @0x%02" HWADDR_PRIx
> + " not implemented\n", size, offset);
> +break;
> +
> +default:
> +ret = 0;
> +qemu_log_mask(LOG_GUEST_ERROR, "ESDHC rd_%ub @0x%02" HWADDR_PRIx
> + " unknown offset\n", size, offset);
> +break;
> +}
> +
> +return ret;
> +}
> +
> +static void esdhci_write(void *opaque, hwaddr offset, uint64_t val,
> + unsigned size)
> +{
> +if (size != 4) {
> +qemu_log_mask(LOG_GUEST_ERROR, "ESDHC wr_%ub @0x%02" HWADDR_PRIx
> + " <- 0x%08lx wrong size\n", size, offset, val);
> +return;
> +}
> +
> +if (offset & 0x3) {
> +qemu_log_mask(LOG_GUEST_ERROR, "ESDHC wr_%ub @0x%02" HWADDR_PRIx
> + " <- 0x%08lx unaligned\n", size, offset, val);
> +return;
> +}
> +
> +switch (offset) {
> +case SDHC_SYSAD:
> +case SDHC_BLKSIZE:
> +case SDHC_ARGUMENT:
> +case SDHC_TRNMOD:
> +case SDHC_BDATA:
> +case SDHC_HOSTCTL:
> +case SDHC_CLKCON:
> +case SDHC_NORINTSTS:
> +case SDHC_NORINTSTSEN:
> +case SDHC_NORINTSIGEN:
> +case SDHC_FEAER:
> +sdhci_write(opaque, offset, val, size);
> +break;
> +
> +case ESDHC_DMA_SYSCTL:
> +case 0x44:
ditto
> +qemu_log_mask(LOG_UNIMP, "ESDHC wr_%ub @0x%02" HWADDR_PRIx " <-
> 0x%08lx "
> + "not implemented\n", size, offset, val);
> +break;
> +
> +default:
> +qemu_log_mask(LOG_GUEST_ERROR, "ESDHC wr_%ub @0x%02" HWADDR_PRIx
> + " <- 0x%08lx unknown offset\n", size, offset, val);
> +break;
> +}
> +}
> +
> +static const MemoryRegionOps esdhc_mmio_ops = {
> +.read = esdhci_read,
> +.write = esdhci_write,
> +.valid = {
> +.min_access_size = 1,
> +.max_access_size = 4,
> +.unaligned = false
> +},
> +.endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static void esdhci_init(Object *obj)
> +{
> +DeviceState *dev = DEVICE(obj);
>
Re: [PATCH 07/11] hw/ppc/e500: Implement pflash handling
On Thu, Sep 15, 2022 at 11:36 PM Bernhard Beschow wrote:
>
> Allows e500 boards to have their root file system reside on flash using
> only builtin devices.
>
> Note that the flash memory area is only created when a -pflash argument is
> given, and that the size is determined by the given file. The idea is to
> put users into control.
>
> Signed-off-by: Bernhard Beschow
> ---
> docs/system/ppc/ppce500.rst | 12 +
> hw/ppc/Kconfig | 1 +
> hw/ppc/e500.c | 54 +
> 3 files changed, 67 insertions(+)
>
> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> index ba6bcb7314..c3f55c6f3d 100644
> --- a/docs/system/ppc/ppce500.rst
> +++ b/docs/system/ppc/ppce500.rst
> @@ -119,6 +119,18 @@ To boot the 32-bit Linux kernel:
>-initrd /path/to/rootfs.cpio \
>-append "root=/dev/ram"
>
> +Rather than using a root file system on ram disk, it is possible to have it
> on
> +emulated flash. Given an ext2 image whose size must be a power of two, it can
> +be used as follows:
> +
> +.. code-block:: bash
> +
> + $ qemu-system-ppc64 -M ppce500 -cpu e500mc -smp 4 -m 2G \
qemu-system-ppc{64|32}
> + -display none -serial stdio \
> + -kernel vmlinux \
> + -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
> + -append "rootwait root=/dev/mtdblock0"
> +
> Running U-Boot
> --
>
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 791fe78a50..769a1ead1c 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -126,6 +126,7 @@ config E500
> select ETSEC
> select GPIO_MPC8XXX
> select OPENPIC
> +select PFLASH_CFI01
> select PLATFORM_BUS
> select PPCE500_PCI
> select SERIAL
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 864b6f3d92..7843a4e04b 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -23,8 +23,10 @@
> #include "e500-ccsr.h"
> #include "net/net.h"
> #include "qemu/config-file.h"
> +#include "hw/block/flash.h"
> #include "hw/char/serial.h"
> #include "hw/pci/pci.h"
> +#include "sysemu/block-backend-io.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/kvm.h"
> #include "sysemu/reset.h"
> @@ -267,6 +269,34 @@ static void sysbus_device_create_devtree(SysBusDevice
> *sbdev, void *opaque)
> }
> }
>
> +static void create_devtree_flash(SysBusDevice *sbdev,
> + PlatformDevtreeData *data)
> +{
> +char *name;
Use g_autofree
> +uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
> + "num-blocks",
> + _fatal);
> +uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
> + "sector-length",
> + _fatal);
> +uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
> + "width",
> + _fatal);
> +hwaddr flashbase = 0;
> +hwaddr flashsize = num_blocks * sector_length;
> +void *fdt = data->fdt;
> +
> +name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
> +qemu_fdt_add_subnode(fdt, name);
> +qemu_fdt_setprop_cell(fdt, name, "#address-cells", 1);
> +qemu_fdt_setprop_cell(fdt, name, "#size-cells", 1);
#address-cells and #size-cells are not needed.
> +qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
> +qemu_fdt_setprop_sized_cells(fdt, name, "reg",
> + 1, flashbase, 1, flashsize);
> +qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
> +g_free(name);
> +}
> +
> static void platform_bus_create_devtree(PPCE500MachineState *pms,
> void *fdt, const char *mpic)
> {
> @@ -276,6 +306,8 @@ static void
> platform_bus_create_devtree(PPCE500MachineState *pms,
> uint64_t addr = pmc->platform_bus_base;
> uint64_t size = pmc->platform_bus_size;
> int irq_start = pmc->platform_bus_first_irq;
> +SysBusDevice *sbdev;
> +bool ambiguous;
>
> /* Create a /platform node that we can put all devices into */
>
> @@ -302,6 +334,13 @@ static void
> platform_bus_create_devtree(PPCE500MachineState *pms,
> /* Loop through all dynamic sysbus devices and create nodes for them */
> foreach_dynamic_sysbus_device(sysbus_device_create_devtree, );
>
> +sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
> +));
Can this be moved into sysbus_device_create_devtree(), and use the
same logic as the eTSEC device?
> +if (sbdev) {
> +assert(!ambiguous);
> +create_devtree_flash(sbdev, );
> +}
> +
> g_free(node);
> }
>
> @@ -856,6 +895,7 @@ void ppce500_init(MachineState *machine)
> unsigned int
Re: [PATCH 09/11] hw/sd/sdhci: Rename ESDHC_* defines to USDHC_*
1 in to
> * bits 5 and 1
> */
> -if (value & ESDHC_CTRL_8BITBUS) {
> +if (value & USDHC_CTRL_8BITBUS) {
> hostctl1 |= SDHC_CTRL_8BITBUS;
> }
>
> -if (value & ESDHC_CTRL_4BITBUS) {
> -hostctl1 |= ESDHC_CTRL_4BITBUS;
> +if (value & USDHC_CTRL_4BITBUS) {
> +hostctl1 |= USDHC_CTRL_4BITBUS;
> }
>
> /*
> @@ -1768,11 +1768,11 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t
> val, unsigned size)
> sdhci_write(opaque, offset, value, size);
> break;
>
> -case ESDHC_MIX_CTRL:
> +case USDHC_MIX_CTRL:
> /*
> * So, when SD/MMC stack in Linux tries to write to "Transfer
> * Mode Register", ESDHC i.MX quirk code will translate it
Here I assume ESDHC i.MX means the Linux eSDHC driver for i.MX, so no
need to replace ESDHC with USDHC?
> - * into a write to ESDHC_MIX_CTRL, so we do the opposite in
> + * into a write to USDHC_MIX_CTRL, so we do the opposite in
> * order to get where we started
> *
> * Note that Auto CMD23 Enable bit is located in a wrong place
> --
Overall LGTM:
Reviewed-by: Bin Meng
Re: [PATCH 08/11] hw/sd/sdhci-internal: Unexport ESDHC defines
On Thu, Sep 15, 2022 at 11:39 PM Bernhard Beschow wrote: > > These defines aren't used outside of sdhci.c, so can be defined there. > > Signed-off-by: Bernhard Beschow > --- > hw/sd/sdhci-internal.h | 20 > hw/sd/sdhci.c | 19 +++ > 2 files changed, 19 insertions(+), 20 deletions(-) > Reviewed-by: Bin Meng
Re: [PATCH 06/11] hw/block/pflash_cfi01: Error out if device length isn't a power of two
On Thu, Sep 15, 2022 at 11:26 PM Bernhard Beschow wrote: > > According to the JEDEC standard the device length is communicated to an > OS as an exponent (power of two). > > Signed-off-by: Bernhard Beschow > --- > hw/block/pflash_cfi01.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > Reviewed-by: Bin Meng
Re: [PATCH 04/11] hw/ppc/mpc8544ds: Add platform bus
On Thu, Sep 15, 2022 at 11:29 PM Bernhard Beschow wrote: > > Models the real device more closely. Please describe the source (e.g.: I assume it's MPC8544DS board manual or something like that?) that describe such memory map for the platform bus. Is this the eLBC bus range that includes the NOR flash device? > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/mpc8544ds.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c > index 81177505f0..cd6cd04bef 100644 > --- a/hw/ppc/mpc8544ds.c > +++ b/hw/ppc/mpc8544ds.c > @@ -14,6 +14,7 @@ > #include "sysemu/device_tree.h" > #include "hw/ppc/openpic.h" > #include "qemu/error-report.h" > +#include "qemu/units.h" > #include "cpu.h" > > static void mpc8544ds_fixup_devtree(void *fdt) > @@ -45,6 +46,11 @@ static void e500plat_machine_class_init(ObjectClass *oc, > void *data) > pmc->pci_nr_slots = 2; > pmc->fixup_devtree = mpc8544ds_fixup_devtree; > pmc->mpic_version = OPENPIC_MODEL_FSL_MPIC_20; > +pmc->has_platform_bus = true; > +pmc->platform_bus_base = 0xEC00ULL; > +pmc->platform_bus_size = 128 * MiB; > +pmc->platform_bus_first_irq = 5; > +pmc->platform_bus_num_irqs = 10; > pmc->ccsrbar_base = 0xE000ULL; > pmc->pci_mmio_base = 0xC000ULL; > pmc->pci_mmio_bus_base = 0xC000ULL; > -- Regards, Bin
Re: [PATCH 05/11] hw/ppc/e500: Remove if statement which is now always true
On Thu, Sep 15, 2022 at 11:34 PM Bernhard Beschow wrote: > > Now that the MPC8544DS board also has a platform bus, the if statement > was always true. > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/e500.c | 30 ++ > hw/ppc/e500.h | 1 - > hw/ppc/e500plat.c | 1 - > hw/ppc/mpc8544ds.c | 1 - > 4 files changed, 14 insertions(+), 19 deletions(-) > Reviewed-by: Bin Meng
Re: [PATCH 03/11] docs/system/ppc/ppce500: Add heading for networking chapter
On Thu, Sep 15, 2022 at 11:29 PM Bernhard Beschow wrote: > > The sudden change of topics is slightly confusing and makes the > networking information less visible. So separate the networking chapter > to improve comprehensibility. > > Signed-off-by: Bernhard Beschow > --- > docs/system/ppc/ppce500.rst | 3 +++ > 1 file changed, 3 insertions(+) > Reviewed-by: Bin Meng
Re: [PATCH 02/11] hw/gpio/meson: Introduce dedicated config switch for hw/gpio/mpc8xxx
On Thu, Sep 15, 2022 at 11:26 PM Bernhard Beschow wrote: > > Having a dedicated config switch makes dependency handling cleaner. > > Signed-off-by: Bernhard Beschow > --- > hw/gpio/Kconfig | 3 +++ > hw/gpio/meson.build | 2 +- > hw/ppc/Kconfig | 1 + > 3 files changed, 5 insertions(+), 1 deletion(-) > Reviewed-by: Bin Meng
Re: [PATCH 01/11] hw/ppc/meson: Allow e500 boards to be enabled separately
On Thu, Sep 15, 2022 at 11:25 PM Bernhard Beschow wrote: > > Gives users more fine-grained control over what should be compiled into > QEMU. > > Signed-off-by: Bernhard Beschow > --- > configs/devices/ppc-softmmu/default.mak | 3 ++- > hw/ppc/Kconfig | 8 > hw/ppc/meson.build | 6 ++ > 3 files changed, 12 insertions(+), 5 deletions(-) > Reviewed-by: Bin Meng
Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
On Thu, Sep 8, 2022 at 9:28 PM Bin Meng wrote: > > At present packaging the required DLLs of QEMU executables is a > manual process, and error prone. > > Improve scripts/nsis.py by adding a logic to automatically package > required DLLs of QEMU executables. > > 'make installer' is tested in the cross-build on Linux in CI, but > not in the Windows native build. Update CI to test the installer > generation on Windows too. > > During testing a 32-bit build issue was exposed in block/nfs.c and > the fix is included in this series. > > > Bin Meng (7): > scripts/nsis.py: Drop the unnecessary path separator > scripts/nsis.py: Fix destination directory name when invoked on > Windows > scripts/nsis.py: Automatically package required DLLs of QEMU > executables > .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build > block/nfs: Fix 32-bit Windows build > .gitlab-ci.d/windows.yml: Unify the prerequisite packages > .gitlab-ci.d/windows.yml: Test 'make installer' in the CI > > meson.build | 1 + > block/nfs.c | 8 ++ > .gitlab-ci.d/windows.yml | 40 --- > scripts/nsis.py | 60 +--- > 4 files changed, 89 insertions(+), 20 deletions(-) > Ping for this series?
[PATCH 5/7] block/nfs: Fix 32-bit Windows build
From: Bin Meng
libnfs.h declares nfs_fstat() as the following for win32:
int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
struct __stat64 *st);
The 'st' parameter should be of type 'struct __stat64'. The
codes happen to build successfully for 64-bit Windows, but it
does not build for 32-bit Windows.
Fixes: 6542aa9c75bc ("block: add native support for NFS")
Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for read-only files")
Signed-off-by: Bin Meng
---
block/nfs.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/nfs.c b/block/nfs.c
index 444c40b458..d5d67937dd 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client,
BlockdevOptionsNfs *opts,
int flags, int open_flags, Error **errp)
{
int64_t ret = -EINVAL;
+#ifdef _WIN32
+struct __stat64 st;
+#else
struct stat st;
+#endif
char *file = NULL, *strp = NULL;
qemu_mutex_init(>mutex);
@@ -781,7 +785,11 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
BlockReopenQueue *queue, Error **errp)
{
NFSClient *client = state->bs->opaque;
+#ifdef _WIN32
+struct __stat64 st;
+#else
struct stat st;
+#endif
int ret = 0;
if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) {
--
2.34.1
[PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
At present packaging the required DLLs of QEMU executables is a manual process, and error prone. Improve scripts/nsis.py by adding a logic to automatically package required DLLs of QEMU executables. 'make installer' is tested in the cross-build on Linux in CI, but not in the Windows native build. Update CI to test the installer generation on Windows too. During testing a 32-bit build issue was exposed in block/nfs.c and the fix is included in this series. Bin Meng (7): scripts/nsis.py: Drop the unnecessary path separator scripts/nsis.py: Fix destination directory name when invoked on Windows scripts/nsis.py: Automatically package required DLLs of QEMU executables .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build block/nfs: Fix 32-bit Windows build .gitlab-ci.d/windows.yml: Unify the prerequisite packages .gitlab-ci.d/windows.yml: Test 'make installer' in the CI meson.build | 1 + block/nfs.c | 8 ++ .gitlab-ci.d/windows.yml | 40 --- scripts/nsis.py | 60 +--- 4 files changed, 89 insertions(+), 20 deletions(-) -- 2.34.1
Re: [PATCH 33/51] tests/qtest: {ahci, ide}-test: Use relative path for temporary files
On Thu, Sep 1, 2022 at 4:58 PM Marc-André Lureau
wrote:
>
>
>
> On Wed, Aug 24, 2022 at 2:55 PM Bin Meng wrote:
>>
>> From: Bin Meng
>>
>> These test cases uses "blkdebug:path/to/config:path/to/image" for
>> testing. On Windows, absolute file paths contain the delimiter ':'
>> which causes the blkdebug filename parser fail to parse filenames.
>>
>
> hmm.. maybe it should learn to escape paths..
>
>
>> Signed-off-by: Bin Meng
>> ---
>>
>> tests/qtest/ahci-test.c | 19 ---
>> tests/qtest/ide-test.c | 18 --
>> 2 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
>> index 0e88cd0eef..bce9ff770c 100644
>> --- a/tests/qtest/ahci-test.c
>> +++ b/tests/qtest/ahci-test.c
>> @@ -1848,7 +1848,7 @@ static void create_ahci_io_test(enum IOMode type, enum
>> AddrMode addr,
>>
>> int main(int argc, char **argv)
>> {
>> -const char *arch;
>> +const char *arch, *base;
>> int ret;
>> int fd;
>> int c;
>> @@ -1886,8 +1886,21 @@ int main(int argc, char **argv)
>> return 0;
>> }
>>
>> +/*
>> + * "base" stores the starting point where we create temporary files.
>> + *
>> + * On Windows, this is set to the relative path of current working
>> + * directory, because the absolute path causes the blkdebug filename
>> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
>> + */
>> +#ifndef _WIN32
>> +base = g_get_tmp_dir();
>> +#else
>> +base = ".";
>> +#endif
>
>
> Meanwhile, that seems reasonable. Perhaps chdir() to the temporary directory
> first? (assuming other paths are absolute)
Other paths in the QEMU command line indeed are absolute, however the
QEMU executable path is set to a relative path from meson.build thus
we cannot chdir() to the temporary directory here.
>
>>
>> +
>> /* Create a temporary image */
>> -tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
>> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
>> fd = mkstemp(tmp_path);
>> g_assert(fd >= 0);
>> if (have_qemu_img()) {
>> @@ -1905,7 +1918,7 @@ int main(int argc, char **argv)
>> close(fd);
>>
>> /* Create temporary blkdebug instructions */
>> -debug_path = g_strdup_printf("%s/qtest-blkdebug.XX",
>> g_get_tmp_dir());
>> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
>> fd = mkstemp(debug_path);
>> g_assert(fd >= 0);
>> close(fd);
>> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
>> index ebbf8e0126..c5cad6c0be 100644
>> --- a/tests/qtest/ide-test.c
>> +++ b/tests/qtest/ide-test.c
>> @@ -1011,17 +1011,31 @@ static void test_cdrom_dma(void)
>>
>> int main(int argc, char **argv)
>> {
>> +const char *base;
>> int fd;
>> int ret;
>>
>> +/*
>> + * "base" stores the starting point where we create temporary files.
>> + *
>> + * On Windows, this is set to the relative path of current working
>> + * directory, because the absolute path causes the blkdebug filename
>> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
>> + */
>> +#ifndef _WIN32
>> +base = g_get_tmp_dir();
>> +#else
>> +base = ".";
>> +#endif
>> +
>> /* Create temporary blkdebug instructions */
>> -debug_path = g_strdup_printf("%s/qtest-blkdebug.XX",
>> g_get_tmp_dir());
>> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
>> fd = mkstemp(debug_path);
>> g_assert(fd >= 0);
>> close(fd);
>>
>> /* Create a temporary raw image */
>> -tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
>> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
>> fd = mkstemp(tmp_path);
>> g_assert(fd >= 0);
>> ret = ftruncate(fd, TEST_IMAGE_SIZE);
>> --
>> 2.34.1
Regards,
Bin
Re: [PATCH 30/51] tests: Skip iotests and qtest when '--without-default-devices'
On Thu, Aug 25, 2022 at 8:04 PM Thomas Huth wrote:
>
> On 24/08/2022 11.40, Bin Meng wrote:
> > From: Bin Meng
> >
> > When QEMU is configured with '--without-default-devices', we should
> > not build and run iotests and qtest because devices used by these
> > test cases are not built in.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > tests/qemu-iotests/meson.build | 5 +
> > tests/qtest/meson.build| 5 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> > index 323a4acb6a..38d9a874d2 100644
> > --- a/tests/qemu-iotests/meson.build
> > +++ b/tests/qemu-iotests/meson.build
> > @@ -2,6 +2,11 @@ if not have_tools or targetos == 'windows' or
> > get_option('gprof')
> > subdir_done()
> > endif
> >
> > +# Skip iotests if configured without a default selection of devices
> > +if not get_option('default_devices')
> > + subdir_done()
> > +endif
> > +
> > foreach cflag: config_host['QEMU_CFLAGS'].split()
> > if cflag.startswith('-fsanitize') and \
> >not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index c97da5a062..0291b3966c 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -4,6 +4,11 @@ if not config_host.has_key('CONFIG_POSIX')
> > subdir_done()
> > endif
> >
> > +# Skip QTests if configured without a default selection of devices
> > +if not get_option('default_devices')
> > + subdir_done()
> > +endif
> > +
> > slow_qtests = {
> > 'ahci-test' : 60,
> > 'bios-tables-test' : 120,
>
> That's a very big hammer already ... I'd prefer if we could work on the
> tests instead to adapt for the availability of devices instead (we've done
> quite a lot of work in this area in the past already, but apparently still
> not enough yet ...)
Adapting tests for the availability of devices is a large scope. I may
not have time to work on this.
I will have to drop this patch in v2, and adjust the patches in the
series to keep bisectability then.
Regards,
Bin
Re: [PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
On Thu, Sep 1, 2022 at 4:42 PM Marc-André Lureau wrote: > > Hi > > On Wed, Aug 24, 2022 at 2:03 PM Bin Meng wrote: >> >> From: Bin Meng >> >> On Windows, the MinGW provided mkstemp() API opens the file with >> exclusive access, denying other processes to read/write the file. >> Such behavior prevents the QEMU executable from opening the file, >> (e.g.: CreateFile returns ERROR_SHARING_VIOLATION). > > > g_mkstemp() doesn't have this behaviour (after running a quick test). Use it? > Thanks for the suggestion! I've switched to using g_file_open_tmp() in patch #7 "tests: Avoid using hardcoded /tmp in test cases", and testing shows that it does not have such an issue. I checked glib sources and see both g_mkstemp() and g_file_open_tmp() call g_open() which allows shared read/write on Windows. So this patch can be dropped. Regards, Bin
Re: [PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
On Thu, Aug 25, 2022 at 8:06 PM Thomas Huth wrote: > > On 24/08/2022 11.40, Bin Meng wrote: > > From: Bin Meng > > > > On Windows, the MinGW provided mkstemp() API opens the file with > > exclusive access, denying other processes to read/write the file. > > Such behavior prevents the QEMU executable from opening the file, > > (e.g.: CreateFile returns ERROR_SHARING_VIOLATION). > > > > This can be fixed by closing the file and reopening it. > > Would it work to use the glib functions instead (like g_file_open_tmp() ?) > Yep, I've switched to using g_file_open_tmp() in patch #7 "tests: Avoid using hardcoded /tmp in test cases", and testing shows that it does not have such an issue. So this patch can be dropped. Regards, Bin
Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation
Hi Marc-André, On Wed, Aug 31, 2022 at 8:54 PM Marc-André Lureau wrote: > > Hi Bin > > On Wed, Aug 24, 2022 at 1:42 PM Bin Meng wrote: >> >> From: Bin Meng >> >> At present get_tmp_filename() has platform specific implementations >> to get the directory to use for temporary files. Switch over to use >> g_get_tmp_dir() which works on all supported platforms. >> > > It "works" quite differently though. Is this patch really necessary here? Without this patch the qtest cases builds on Windows do not have any problem. So it is optional. I put it in the same series as it has the same context of using hardcoded /tmp directory name. > > If yes, please explain why. > > If not, I suggest you drop optional / rfc / "nice to have" patches from the > series. It will help to get it merged faster. I can drop this single patch and send another single patch if this is the desired practice. > > thanks Regards, Bin
[PATCH 33/51] tests/qtest: {ahci, ide}-test: Use relative path for temporary files
From: Bin Meng
These test cases uses "blkdebug:path/to/config:path/to/image" for
testing. On Windows, absolute file paths contain the delimiter ':'
which causes the blkdebug filename parser fail to parse filenames.
Signed-off-by: Bin Meng
---
tests/qtest/ahci-test.c | 19 ---
tests/qtest/ide-test.c | 18 --
2 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 0e88cd0eef..bce9ff770c 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1848,7 +1848,7 @@ static void create_ahci_io_test(enum IOMode type, enum
AddrMode addr,
int main(int argc, char **argv)
{
-const char *arch;
+const char *arch, *base;
int ret;
int fd;
int c;
@@ -1886,8 +1886,21 @@ int main(int argc, char **argv)
return 0;
}
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create a temporary image */
-tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
fd = mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
@@ -1905,7 +1918,7 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
-debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", g_get_tmp_dir());
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
fd = mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index ebbf8e0126..c5cad6c0be 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -1011,17 +1011,31 @@ static void test_cdrom_dma(void)
int main(int argc, char **argv)
{
+const char *base;
int fd;
int ret;
+/*
+ * "base" stores the starting point where we create temporary files.
+ *
+ * On Windows, this is set to the relative path of current working
+ * directory, because the absolute path causes the blkdebug filename
+ * parser fail to parse "blkdebug:path/to/config:path/to/image".
+ */
+#ifndef _WIN32
+base = g_get_tmp_dir();
+#else
+base = ".";
+#endif
+
/* Create temporary blkdebug instructions */
-debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", g_get_tmp_dir());
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
fd = mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
/* Create a temporary raw image */
-tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
+tmp_path = g_strdup_printf("%s/qtest.XX", base);
fd = mkstemp(tmp_path);
g_assert(fd >= 0);
ret = ftruncate(fd, TEST_IMAGE_SIZE);
--
2.34.1
[PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
From: Bin Meng
On Windows, the MinGW provided mkstemp() API opens the file with
exclusive access, denying other processes to read/write the file.
Such behavior prevents the QEMU executable from opening the file,
(e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
This can be fixed by closing the file and reopening it.
Signed-off-by: Bin Meng
---
tests/qtest/ahci-test.c| 14 ++
tests/qtest/boot-serial-test.c | 13 +
2 files changed, 27 insertions(+)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index f26cd6f86f..0e88cd0eef 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1443,6 +1443,20 @@ static int prepare_iso(size_t size, unsigned char **buf,
char **name)
int fd = mkstemp(cdrom_path);
g_assert(fd != -1);
+#ifdef _WIN32
+/*
+ * On Windows, the MinGW provided mkstemp() API opens the file with
+ * exclusive access, denying other processes to read/write the file.
+ * Such behavior prevents the QEMU executable from opening the file,
+ * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
+ *
+ * Close the file and reopen it.
+ */
+close(fd);
+fd = open(cdrom_path, O_WRONLY);
+g_assert(fd != -1);
+#endif
+
g_assert(buf);
g_assert(name);
patt = g_malloc(size);
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 404adcfa20..fb6c81bf35 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -235,6 +235,19 @@ static void test_machine(const void *data)
ser_fd = mkstemp(serialtmp);
g_assert(ser_fd != -1);
+#ifdef _WIN32
+/*
+ * On Windows, the MinGW provided mkstemp() API opens the file with
+ * exclusive access, denying other processes to read/write the file.
+ * Such behavior prevents the QEMU executable from opening the file,
+ * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
+ *
+ * Close the file and reopen it.
+ */
+close(ser_fd);
+ser_fd = open(serialtmp, O_RDONLY);
+g_assert(ser_fd != -1);
+#endif
if (test->kernel) {
code = test->kernel;
--
2.34.1
[PATCH 30/51] tests: Skip iotests and qtest when '--without-default-devices'
From: Bin Meng
When QEMU is configured with '--without-default-devices', we should
not build and run iotests and qtest because devices used by these
test cases are not built in.
Signed-off-by: Bin Meng
---
tests/qemu-iotests/meson.build | 5 +
tests/qtest/meson.build| 5 +
2 files changed, 10 insertions(+)
diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 323a4acb6a..38d9a874d2 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -2,6 +2,11 @@ if not have_tools or targetos == 'windows' or
get_option('gprof')
subdir_done()
endif
+# Skip iotests if configured without a default selection of devices
+if not get_option('default_devices')
+ subdir_done()
+endif
+
foreach cflag: config_host['QEMU_CFLAGS'].split()
if cflag.startswith('-fsanitize') and \
not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c97da5a062..0291b3966c 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -4,6 +4,11 @@ if not config_host.has_key('CONFIG_POSIX')
subdir_done()
endif
+# Skip QTests if configured without a default selection of devices
+if not get_option('default_devices')
+ subdir_done()
+endif
+
slow_qtests = {
'ahci-test' : 60,
'bios-tables-test' : 120,
--
2.34.1
[PATCH 38/51] tests/qtest: {ahci,ide}-test: Open file in binary mode
From: Xuzhou Cheng By default Windows opens file in text mode, while a POSIX compliant implementation treats text files and binary files the same. The fopen() 'mode' string can include the letter 'b' to indicate binary mode shall be used. POSIX spec says the character 'b' shall have no effect, but is allowed for ISO C standard conformance. Let's add the letter 'b' which works on both POSIX and Windows. Similar situation applies to the open() 'flags' where O_BINARY is used for binary mode. Signed-off-by: Xuzhou Cheng Signed-off-by: Bin Meng --- tests/qtest/ahci-test.c | 2 +- tests/qtest/ide-test.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c index bce9ff770c..be11508c75 100644 --- a/tests/qtest/ahci-test.c +++ b/tests/qtest/ahci-test.c @@ -1453,7 +1453,7 @@ static int prepare_iso(size_t size, unsigned char **buf, char **name) * Close the file and reopen it. */ close(fd); -fd = open(cdrom_path, O_WRONLY); +fd = open(cdrom_path, O_WRONLY | O_BINARY); g_assert(fd != -1); #endif diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c index c5cad6c0be..ee03dea4fa 100644 --- a/tests/qtest/ide-test.c +++ b/tests/qtest/ide-test.c @@ -892,7 +892,7 @@ static void cdrom_pio_impl(int nblocks) /* Prepopulate the CDROM with an interesting pattern */ generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh); g_assert_cmpint(ret, ==, patt_blocks); fclose(fh); @@ -993,7 +993,7 @@ static void test_cdrom_dma(void) prdt[0].size = cpu_to_le32(len | PRDT_EOT); generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE); -fh = fopen(tmp_path, "w+"); +fh = fopen(tmp_path, "wb+"); ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh); g_assert_cmpint(ret, ==, 16); fclose(fh); -- 2.34.1
[PATCH 08/51] block/vvfat: Unify the mkdir() call
From: Bin Meng
There is a difference in the mkdir() call for win32 and non-win32
platforms, and currently is handled in the codes with #ifdefs.
glib provides a portable g_mkdir_with_parents() API and we can use
it to unify the codes without #ifdefs.
Signed-off-by: Bin Meng
---
block/vvfat.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..9c389ce5ea 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2726,13 +2726,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
mapping_t* mapping;
int j, parent_path_len;
-#ifdef __MINGW32__
-if (mkdir(commit->path))
+if (g_mkdir_with_parents(commit->path, 0755)) {
return -5;
-#else
-if (mkdir(commit->path, 0755))
-return -5;
-#endif
+}
mapping = insert_mapping(s, commit->param.mkdir.cluster,
commit->param.mkdir.cluster + 1);
--
2.34.1
[PATCH 03/51] block: Unify the get_tmp_filename() implementation
From: Bin Meng
At present get_tmp_filename() has platform specific implementations
to get the directory to use for temporary files. Switch over to use
g_get_tmp_dir() which works on all supported platforms.
Signed-off-by: Bin Meng
---
block.c | 16 ++--
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index bc85f46eed..d06df47f72 100644
--- a/block.c
+++ b/block.c
@@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry
*geo)
*/
int get_tmp_filename(char *filename, int size)
{
-#ifdef _WIN32
-char temp_dir[MAX_PATH];
-/* GetTempFileName requires that its output buffer (4th param)
- have length MAX_PATH or greater. */
-assert(size >= MAX_PATH);
-return (GetTempPath(MAX_PATH, temp_dir)
-&& GetTempFileName(temp_dir, "qem", 0, filename)
-? 0 : -GetLastError());
-#else
int fd;
const char *tmpdir;
-tmpdir = getenv("TMPDIR");
-if (!tmpdir) {
-tmpdir = "/var/tmp";
-}
+tmpdir = g_get_tmp_dir();
+
if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
return -EOVERFLOW;
}
@@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
return -errno;
}
return 0;
-#endif
}
/*
--
2.34.1
[PATCH 00/51] tests/qtest: Enable running qtest on Windows
In prepartion to adding virtio-9p support on Windows, this series
enables running qtest on Windows, so that we can run the virtio-9p
tests on Windows to make sure it does not break accidently.
Patch 1-22 updates various components (mostly test cases) so that
they can build on Windows with the same functionality.
Patch 23 supports the qtest accelerator for Windows.
Patch 31 updates the libqtest core for Windows.
Patch 32-47 are the fixes of qtest errors exposed when running on Windows.
Patch 49 fixes the instability of running qtests on Windows.
Patch 50 updates the CI to run qtests on Windows.
Patch 51 documents best practices of writing portable test cases as
we learned during the enablement of running qtest on Windows.
Based-on: <20220802075200.907360-1-bmeng...@gmail.com>
Bin Meng (41):
tests/qtest: Use g_setenv()
tests/qtest: Use g_mkdtemp()
block: Unify the get_tmp_filename() implementation
semihosting/arm-compat-semi: Avoid using hardcoded /tmp
tcg: Avoid using hardcoded /tmp
util/qemu-sockets: Use g_get_tmp_dir() to get the directory for
temporary files
tests: Avoid using hardcoded /tmp in test cases
block/vvfat: Unify the mkdir() call
fsdev/virtfs-proxy-helper: Use g_mkdir_with_parents()
hw/usb: dev-mtp: Use g_mkdir_with_parents()
qga/commands-posix-ssh: Use g_mkdir_with_parents()
tests: Use g_mkdir_with_parents()
tests/qtest: migration-test: Handle link() for win32
backends/tpm: Exclude headers and macros that don't exist on win32
tests/qtest: Adapt {m48t59,rtc}-test cases for win32
tests/qtest: Build e1000e-test for posix only
tests/qtest: Build virtio-net-test for posix only
tests/qtest: Build cases that use memory-backend-file for posix only
tests/qtest: Build test-filter-{mirror,redirector} cases for posix
only
tests/qtest: i440fx-test: Skip running request_{bios,pflash} for win32
tests/qtest: migration-test: Skip running test_migrate_fd_proto on
win32
tests/qtest: qmp-test: Skip running test_qmp_oob for win32
tests/qtest: libqtest: Exclude the *_fds APIs for win32
tests/qtest: libqtest: Install signal handler via signal()
tests: Skip iotests and qtest when '--without-default-devices'
tests/qtest: Support libqtest to build and run on Windows
tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
tests/qtest: {ahci,ide}-test: Use relative path for temporary files
tests/qtest: bios-tables-test: Adapt the case for win32
tests/qtest: device-plug-test: Reverse the usage of double/single
quotes
tests/qtest: machine-none-test: Use double quotes to pass the cpu
option
tests/qtest: migration-test: Disable IO redirection for win32
tests/qtest: npcm7xx_emc-test: Skip running test_{tx,rx} on win32
tests/qtest: microbit-test: Fix socket access for win32
tests/qtest: prom-env-test: Use double quotes to pass the prom-env
option
tests/qtest: libqtest: Replace the call to close a socket with
closesocket()
tests/qtest: libqtest: Correct the timeout unit of blocking receive
calls for win32
io/channel-watch: Drop a superfluous '#ifdef WIN32'
io/channel-watch: Fix socket watch on Windows
.gitlab-ci.d/windows.yml: Increase the timeout to the runner limit
docs/devel: testing: Document writing portable test cases
Xuzhou Cheng (10):
accel/qtest: Support qtest accelerator for Windows
tests/qtest: libqos: Drop inclusion of
tests/qtest: libqos: Rename malloc.h to libqos-malloc.h
tests/qtest: libqtest: Move global_qtest definition back to libqtest.c
tests/qtest: Use send/recv for socket communication
tests/qtest: {ahci,ide}-test: Open file in binary mode
tests/qtest: virtio-net-failover: Disable migration tests for win32
chardev/char-file: Add FILE_SHARE_WRITE when openning the file for
win32
tests/qtest: migration-test: Kill "to" after migration is canceled
hw/ppc: spapr: Use qemu_vfree() to free spapr->htab
docs/devel/testing.rst| 30
backends/tpm/tpm_ioctl.h | 4 +
include/hw/core/cpu.h | 1 +
tests/qtest/fuzz/generic_fuzz_configs.h | 8 +-
tests/qtest/libqos/generic-pcihost.h | 2 +-
.../libqos/{malloc.h => libqos-malloc.h} | 0
tests/qtest/libqos/libqos.h | 2 +-
tests/qtest/libqos/malloc-pc.h| 2 +-
tests/qtest/libqos/malloc-spapr.h | 2 +-
tests/qtest/libqos/pci-pc.h | 2 +-
tests/qtest/libqos/pci-spapr.h| 2 +-
tests/qtest/libqos/qgraph.h | 2 +-
tests/qtest/libqos/qos_external.h | 2 +-
tests/qtest/libqos/rtas.h | 2 +-
tests/qtest/libqos/virtio.h | 2 +-
tests/qtest/libqtest-single.h | 2 +-
tests/qtest/libqtest.h| 8 +
tests/qtest/migration-helpers.h | 2 +
accel/dummy-cpus.c
[PATCH 07/51] tests: Avoid using hardcoded /tmp in test cases
From: Bin Meng
Use g_get_tmp_dir() to get the directory to use for temporary files.
Signed-off-by: Bin Meng
---
tests/qtest/fuzz/generic_fuzz_configs.h | 6 --
tests/qtest/ahci-test.c | 15 +++
tests/qtest/aspeed_smc-test.c | 4 +++-
tests/qtest/boot-serial-test.c | 8 ++--
tests/qtest/cxl-test.c | 9 ++---
tests/qtest/fdc-test.c | 4 +++-
tests/qtest/fuzz/virtio_blk_fuzz.c | 2 +-
tests/qtest/hd-geo-test.c | 8
tests/qtest/ide-test.c | 8 ++--
tests/qtest/libqtest.c | 10 +++---
tests/qtest/migration-test.c| 4 +++-
tests/qtest/pflash-cfi02-test.c | 7 +--
tests/qtest/qmp-test.c | 4 +++-
tests/qtest/vhost-user-blk-test.c | 3 ++-
tests/qtest/vhost-user-test.c | 3 ++-
tests/qtest/virtio-blk-test.c | 2 +-
tests/qtest/virtio-scsi-test.c | 3 ++-
tests/unit/test-image-locking.c | 6 --
tests/unit/test-qga.c | 2 +-
tests/vhost-user-bridge.c | 3 ++-
20 files changed, 76 insertions(+), 35 deletions(-)
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 0775e6702b..d0f9961187 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -20,13 +20,15 @@ typedef struct generic_fuzz_config {
} generic_fuzz_config;
static inline gchar *generic_fuzzer_virtio_9p_args(void){
-char tmpdir[] = "/tmp/qemu-fuzz.XX";
+char *tmpdir = g_strdup_printf("%s/qemu-fuzz.XX", g_get_tmp_dir());
g_assert_nonnull(g_mkdtemp(tmpdir));
-return g_strdup_printf("-machine q35 -nodefaults "
+gchar *args = g_strdup_printf("-machine q35 -nodefaults "
"-device virtio-9p,fsdev=hshare,mount_tag=hshare "
"-fsdev local,id=hshare,path=%s,security_model=mapped-xattr,"
"writeout=immediate,fmode=0600,dmode=0700", tmpdir);
+g_free(tmpdir);
+return args;
}
const generic_fuzz_config predefined_configs[] = {
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index f1e510b0ac..f26cd6f86f 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -44,9 +44,9 @@
#define TEST_IMAGE_SIZE_MB_SMALL 64
/*** Globals ***/
-static char tmp_path[] = "/tmp/qtest.XX";
-static char debug_path[] = "/tmp/qtest-blkdebug.XX";
-static char mig_socket[] = "/tmp/qtest-migration.XX";
+static char *tmp_path;
+static char *debug_path;
+static char *mig_socket;
static bool ahci_pedantic;
static const char *imgfmt;
static unsigned test_image_size_mb;
@@ -1437,7 +1437,7 @@ static void test_ncq_simple(void)
static int prepare_iso(size_t size, unsigned char **buf, char **name)
{
-char cdrom_path[] = "/tmp/qtest.iso.XX";
+char *cdrom_path = g_strdup_printf("%s/qtest.iso.XX", g_get_tmp_dir());
unsigned char *patt;
ssize_t ret;
int fd = mkstemp(cdrom_path);
@@ -1454,6 +1454,7 @@ static int prepare_iso(size_t size, unsigned char **buf,
char **name)
*name = g_strdup(cdrom_path);
*buf = patt;
+g_free(cdrom_path);
return fd;
}
@@ -1872,6 +1873,7 @@ int main(int argc, char **argv)
}
/* Create a temporary image */
+tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
fd = mkstemp(tmp_path);
g_assert(fd >= 0);
if (have_qemu_img()) {
@@ -1889,11 +1891,13 @@ int main(int argc, char **argv)
close(fd);
/* Create temporary blkdebug instructions */
+debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", g_get_tmp_dir());
fd = mkstemp(debug_path);
g_assert(fd >= 0);
close(fd);
/* Reserve a hollow file to use as a socket for migration tests */
+mig_socket = g_strdup_printf("%s/qtest-migration.XX", g_get_tmp_dir());
fd = mkstemp(mig_socket);
g_assert(fd >= 0);
close(fd);
@@ -1947,8 +1951,11 @@ int main(int argc, char **argv)
/* Cleanup */
unlink(tmp_path);
+g_free(tmp_path);
unlink(debug_path);
+g_free(debug_path);
unlink(mig_socket);
+g_free(mig_socket);
return ret;
}
diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index 05ce941566..cab769459c 100644
--- a/tests/qtest/aspeed_smc-test.c
+++ b/tests/qtest/aspeed_smc-test.c
@@ -608,7 +608,7 @@ static void test_write_block_protect_bottom_bit(void)
flash_reset();
}
-static char tmp_path[] = "/tmp/qtest.m25p80.XX";
+static char *tmp_path;
int main(int argc, char **argv)
{
@@ -617,6 +617,7 @@ int main(int argc, char **argv)
g_test_init(, , NULL);
+tmp_path = g_strdup_printf("%s/qtest.m25p80.XX", g_get_tmp_dir());
fd
[PATCH 02/51] tests/qtest: Use g_mkdtemp()
From: Bin Meng
Windows does not provide a mkdtemp() API, but glib does.
Replace mkdtemp() call with the glib version.
Signed-off-by: Bin Meng
---
tests/qtest/fuzz/generic_fuzz_configs.h | 2 +-
tests/qtest/cdrom-test.c| 2 +-
tests/qtest/cxl-test.c | 6 +++---
tests/qtest/ivshmem-test.c | 4 ++--
tests/qtest/libqos/virtio-9p.c | 4 ++--
tests/qtest/libqtest.c | 2 +-
tests/qtest/migration-test.c| 4 ++--
tests/qtest/qmp-test.c | 4 ++--
tests/qtest/vhost-user-test.c | 4 ++--
tests/unit/test-qga.c | 2 +-
10 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 004c701915..0775e6702b 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -21,7 +21,7 @@ typedef struct generic_fuzz_config {
static inline gchar *generic_fuzzer_virtio_9p_args(void){
char tmpdir[] = "/tmp/qemu-fuzz.XX";
-g_assert_nonnull(mkdtemp(tmpdir));
+g_assert_nonnull(g_mkdtemp(tmpdir));
return g_strdup_printf("-machine q35 -nodefaults "
"-device virtio-9p,fsdev=hshare,mount_tag=hshare "
diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index a7766a9e65..26a2400181 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -52,7 +52,7 @@ static int prepare_image(const char *arch, char *isoimage)
perror("Error creating temporary iso image file");
return -1;
}
-if (!mkdtemp(srcdir)) {
+if (!g_mkdtemp(srcdir)) {
perror("Error creating temporary directory");
goto cleanup;
}
diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index 2133e973f4..4e6d285061 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -95,7 +95,7 @@ static void cxl_t3d(void)
char template[] = "/tmp/cxl-test-XX";
const char *tmpfs;
-tmpfs = mkdtemp(template);
+tmpfs = g_mkdtemp(template);
g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D, tmpfs, tmpfs);
@@ -109,7 +109,7 @@ static void cxl_1pxb_2rp_2t3d(void)
char template[] = "/tmp/cxl-test-XX";
const char *tmpfs;
-tmpfs = mkdtemp(template);
+tmpfs = g_mkdtemp(template);
g_string_printf(cmdline, QEMU_PXB_CMD QEMU_2RP QEMU_2T3D,
tmpfs, tmpfs, tmpfs, tmpfs);
@@ -124,7 +124,7 @@ static void cxl_2pxb_4rp_4t3d(void)
char template[] = "/tmp/cxl-test-XX";
const char *tmpfs;
-tmpfs = mkdtemp(template);
+tmpfs = g_mkdtemp(template);
g_string_printf(cmdline, QEMU_2PXB_CMD QEMU_4RP QEMU_4T3D,
tmpfs, tmpfs, tmpfs, tmpfs, tmpfs, tmpfs,
diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
index e23a97fa8e..9611d05eb5 100644
--- a/tests/qtest/ivshmem-test.c
+++ b/tests/qtest/ivshmem-test.c
@@ -481,8 +481,8 @@ int main(int argc, char **argv)
tmpshmem = mmap(0, TMPSHMSIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
g_assert(tmpshmem != MAP_FAILED);
/* server */
-if (mkdtemp(dir) == NULL) {
-g_error("mkdtemp: %s", g_strerror(errno));
+if (g_mkdtemp(dir) == NULL) {
+g_error("g_mkdtemp: %s", g_strerror(errno));
}
tmpdir = dir;
tmpserver = g_strconcat(tmpdir, "/server", NULL);
diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index 70aea8bf62..ae9b0a20e2 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -48,9 +48,9 @@ void virtio_9p_create_local_test_dir(void)
*/
char *template = concat_path(pwd, "qtest-9p-local-XX");
-local_test_path = mkdtemp(template);
+local_test_path = g_mkdtemp(template);
if (!local_test_path) {
-g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
+g_test_message("g_mkdtemp('%s') failed: %s", template,
strerror(errno));
}
g_assert(local_test_path != NULL);
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index ad6860d774..7c9fc07de4 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -393,7 +393,7 @@ QTestState *qtest_init_with_serial(const char *extra_args,
int *sock_fd)
char *sock_path, sock_dir[] = "/tmp/qtest-serial-XX";
QTestState *qts;
-g_assert_true(mkdtemp(sock_dir) != NULL);
+g_assert_true(g_mkdtemp(sock_dir) != NULL);
sock_path = g_strdup_printf("%s/sock", sock_dir);
sock_fd_init = init_socket(sock_path);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 520a5f917c..52988b86eb 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2450,9 +2450,9 @@ int main(int argc, char **ar
[PATCH v3 3/3] util/aio-win32: Correct the event array size in aio_poll()
From: Bin Meng
WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
object handles. Correct the event array size in aio_poll() and
add a assert() to ensure it does not cause out of bound access.
Signed-off-by: Bin Meng
Reviewed-by: Stefan Weil
Reviewed-by: Marc-André Lureau
---
(no changes since v2)
Changes in v2:
- change 'count' to unsigned
util/aio-win32.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 44003d645e..80cfe012ad 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -326,9 +326,9 @@ void aio_dispatch(AioContext *ctx)
bool aio_poll(AioContext *ctx, bool blocking)
{
AioHandler *node;
-HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
+HANDLE events[MAXIMUM_WAIT_OBJECTS];
bool progress, have_select_revents, first;
-int count;
+unsigned count;
int timeout;
/*
@@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
QLIST_FOREACH_RCU(node, >aio_handlers, node) {
if (!node->deleted && node->io_notify
&& aio_node_check(ctx, node->is_external)) {
+assert(count < MAXIMUM_WAIT_OBJECTS);
events[count++] = event_notifier_get_handle(node->e);
}
}
--
2.34.1
[PATCH v2 2/2] util/aio-win32: Correct the event array size in aio_poll()
From: Bin Meng
WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
object handles. Correct the event array size in aio_poll() and
add a assert() to ensure it does not cause out of bound access.
Signed-off-by: Bin Meng
Reviewed-by: Stefan Weil
---
Changes in v2:
- change 'count' to unsigned
util/aio-win32.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 44003d645e..80cfe012ad 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -326,9 +326,9 @@ void aio_dispatch(AioContext *ctx)
bool aio_poll(AioContext *ctx, bool blocking)
{
AioHandler *node;
-HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
+HANDLE events[MAXIMUM_WAIT_OBJECTS];
bool progress, have_select_revents, first;
-int count;
+unsigned count;
int timeout;
/*
@@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
QLIST_FOREACH_RCU(node, >aio_handlers, node) {
if (!node->deleted && node->io_notify
&& aio_node_check(ctx, node->is_external)) {
+assert(count < MAXIMUM_WAIT_OBJECTS);
events[count++] = event_notifier_get_handle(node->e);
}
}
--
2.34.1
Re: [PATCH 2/2] util/aio-win32: Correct the event array size in aio_poll()
On Fri, Aug 5, 2022 at 11:09 PM Stefan Weil wrote:
>
> Am 05.08.22 um 16:56 schrieb Bin Meng:
>
> > From: Bin Meng
> >
> > WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
> > object handles. Correct the event array size in aio_poll() and
> > add a assert() to ensure it does not cause out of bound access.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > util/aio-win32.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/aio-win32.c b/util/aio-win32.c
> > index 44003d645e..8cf5779567 100644
> > --- a/util/aio-win32.c
> > +++ b/util/aio-win32.c
> > @@ -326,7 +326,7 @@ void aio_dispatch(AioContext *ctx)
> > bool aio_poll(AioContext *ctx, bool blocking)
> > {
> > AioHandler *node;
> > -HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
> > +HANDLE events[MAXIMUM_WAIT_OBJECTS];
> > bool progress, have_select_revents, first;
> > int count;
> > int timeout;
> > @@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
> > QLIST_FOREACH_RCU(node, >aio_handlers, node) {
> > if (!node->deleted && node->io_notify
> > && aio_node_check(ctx, node->is_external)) {
> > +assert(count < MAXIMUM_WAIT_OBJECTS);
>
>
> Would using g_assert for new code be better? Currently the rest of that
> file (and most QEMU code) uses assert.
Yeah, I noticed that but didn't do that because I feel it's better to
be consistent, at least in this single file.
Changing to g_assert() could be a future patch, if necessary.
>
> count could also be changed from int to unsigned (which matches better
> to the unsigned DWORD).
>
changed in v2.
> Reviewed-by: Stefan Weil
Thanks!
Regards,
Bin
[PATCH 2/2] util/aio-win32: Correct the event array size in aio_poll()
From: Bin Meng
WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
object handles. Correct the event array size in aio_poll() and
add a assert() to ensure it does not cause out of bound access.
Signed-off-by: Bin Meng
---
util/aio-win32.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 44003d645e..8cf5779567 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -326,7 +326,7 @@ void aio_dispatch(AioContext *ctx)
bool aio_poll(AioContext *ctx, bool blocking)
{
AioHandler *node;
-HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
+HANDLE events[MAXIMUM_WAIT_OBJECTS];
bool progress, have_select_revents, first;
int count;
int timeout;
@@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
QLIST_FOREACH_RCU(node, >aio_handlers, node) {
if (!node->deleted && node->io_notify
&& aio_node_check(ctx, node->is_external)) {
+assert(count < MAXIMUM_WAIT_OBJECTS);
events[count++] = event_notifier_get_handle(node->e);
}
}
--
2.34.1
Re: [PATCH] hw/sd/sdcard: Return ILLEGAL for CMD19/CMD23 prior SD spec v3.01
On Mon, May 9, 2022 at 10:13 PM Philippe Mathieu-Daudé
wrote:
>
> From: Philippe Mathieu-Daudé
>
> CMD19 (SEND_TUNING_BLOCK) and CMD23 (SET_BLOCK_COUNT) were
> added in the Physical SD spec v3.01. When earlier spec version
nits: it should be spec v3.00, despite the fact that in QEMU we have
been using a name v3.01 to indicate v3.00.
> is requested, we should return ILLEGAL.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 8e6fa09151..7e3bb12b1a 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1263,7 +1263,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
>
> case 19:/* CMD19: SEND_TUNING_BLOCK (SD) */
> if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> -break;
> +goto bad_cmd;
> }
> if (sd->state == sd_transfer_state) {
> sd->state = sd_sendingdata_state;
> @@ -1274,7 +1274,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
>
> case 23:/* CMD23: SET_BLOCK_COUNT */
> if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> -break;
> + goto bad_cmd;
> }
> switch (sd->state) {
> case sd_transfer_state:
> --
Reviewed-by: Bin Meng
Re: [PATCH] hw/sd: sdhci: Enable 64-bit system bus capability in the default SD/MMC host controller
On Thu, Jun 24, 2021 at 3:01 AM Joanne Koong wrote: > > The default SD/MMC host controller uses SD spec v2.00. 64-bit system bus > capability > was added in v2. > > In this change, we arrive at 0x157834b4 by computing (0x057834b4 | (1ul << > 28)) > where 28 represents the BUS64BIT SDHC_CAPAB field. > > Signed-off-by: Joanne Koong > --- > hw/sd/sdhci-internal.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Bin Meng
Re: [PATCH 3/3] hw/sd: Check for valid address range in SEND_WRITE_PROT (CMD30)
On Fri, Jul 2, 2021 at 11:59 PM Philippe Mathieu-Daudé wrote:
>
> OSS-Fuzz found sending illegal addresses when querying the write
> protection bits triggers an assertion:
>
> qemu-fuzz-i386: hw/sd/sd.c:824: uint32_t sd_wpbits(SDState *, uint64_t):
> Assertion `wpnum < sd->wpgrps_size' failed.
> ==11578== ERROR: libFuzzer: deadly signal
> #8 0x7628e091 in __assert_fail
> #9 0x588f1a3c in sd_wpbits hw/sd/sd.c:824:9
> #10 0x588dd271 in sd_normal_command hw/sd/sd.c:1383:38
> #11 0x588d777c in sd_do_command hw/sd/sd.c
> #12 0x58cb25a0 in sdbus_do_command hw/sd/core.c:100:16
> #13 0x58e02a9a in sdhci_send_command hw/sd/sdhci.c:337:12
> #14 0x58dffa46 in sdhci_write hw/sd/sdhci.c:1187:9
> #15 0x598b9d76 in memory_region_write_accessor softmmu/memory.c:489:5
>
> Similarly to commit 8573378e62d ("hw/sd: fix out-of-bounds check
> for multi block reads"), check the address range before sending
> the status of the write protection bits.
>
> Include the qtest reproducer provided by Alexander Bulekov:
>
> $ make check-qtest-i386
> ...
> Running test qtest-i386/fuzz-sdcard-test
> qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum <
> sd->wpgrps_size' failed.
>
> Reported-by: OSS-Fuzz (Issue 29225)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/450
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sd.c | 5 +++
> tests/qtest/fuzz-sdcard-test.c | 66 ++
> MAINTAINERS| 3 +-
> tests/qtest/meson.build | 1 +
> 4 files changed, 74 insertions(+), 1 deletion(-)
> create mode 100644 tests/qtest/fuzz-sdcard-test.c
>
Reviewed-by: Bin Meng
Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler
On Sat, Jun 26, 2021 at 1:17 AM Philippe Mathieu-Daudé wrote:
>
> On 6/25/21 3:49 PM, Bin Meng wrote:
> > On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé
> > wrote:
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé
> >> ---
> >> hw/sd/sd.c | 21 -
> >> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> >> qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n",
> >> req.cmd);
> >> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
> >> [26]= sd_cmd_illegal,
> >> [52 ... 54] = sd_cmd_illegal,
> >> },
> >> +.cmd = {
> >> +[6] = sd_cmd_unimplemented,
> >> +},
> >> };
> >
> > Does this compile?
>
> Yes.
>
> > Or is this another GCC extension to C?
>
> I haven't checked when this was introduced, but QEMU uses it since
> quite some time now.
>
> Apparently this is:
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
Yep, I know designated initialization of a C array, but I don't know
gcc does not complain two .cmd here
>
> "In ISO C99 you can give the elements in any order, specifying
> the array indices or structure field names they apply to, and
> GNU C allows this as an extension in C90 mode as well."
>
> > But I think you wanted to say .acmd = ?
>
> Oops!
>
> Thanks for the review,
Regards,
Bin
