Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 2023-09-18 오후 1:41, Jeuk Kim wrote: On 2023-09-15 16:59, Paolo Bonzini wrote: On 9/15/23 00:19, Jeuk Kim wrote: First, ufs-lu has a feature called "unit descriptor". This feature shows the status of the ufs-lu and only works with UFS-specific "query request" commands, not SCSI commands. This looks like something that can be implemented in the UFS subsystem. UFS also has something called a well-known lu. Unlike typical SCSI devices, where each lu is independent, UFS can control other lu's through the well-known lu. This can also be implemented in UfsBus. Finally, UFS-LU will have features that SCSI-HD does not have, such as the zone block command. These should be implemented in scsi-hd as well. In addition to this, I wanted some scsi commands to behave differently from scsi-hd, for example, the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK", and the mode_sense_page command should have a different result. Some of these don't have much justification, and others (such as the control page) could be done in scsi-hd as well. We should look into cleaning this up and making ufs-lu share a lot more code with scsi-hd; possibly even supporting -device scsi-hd with UFS devices. I am not going to ask you for a revert, but if this is not done before 8.2 is out, I will ask you to disable it by default in hw/ufs/Kconfig. In the future, please Cc the SCSI maintainers for UFS patches. Paolo Thanks for the comment. ufs-lu took most of its code from scsi-hd, so I completely agree that we should make scsi-hd code shareable to reduce code redundancy and make it better. Sorry about it. This sentence is misleading. I meant to say "I completely agree that ufs-lu should be made to reuse scsi-hd code to reduce code redundancy and improve code quality." I will fix it and get back to you soon. Thank you. Sincerely, Jeuk Thanks
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 2023-09-15 16:59, Paolo Bonzini wrote: On 9/15/23 00:19, Jeuk Kim wrote: First, ufs-lu has a feature called "unit descriptor". This feature shows the status of the ufs-lu and only works with UFS-specific "query request" commands, not SCSI commands. This looks like something that can be implemented in the UFS subsystem. UFS also has something called a well-known lu. Unlike typical SCSI devices, where each lu is independent, UFS can control other lu's through the well-known lu. This can also be implemented in UfsBus. Finally, UFS-LU will have features that SCSI-HD does not have, such as the zone block command. These should be implemented in scsi-hd as well. In addition to this, I wanted some scsi commands to behave differently from scsi-hd, for example, the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK", and the mode_sense_page command should have a different result. Some of these don't have much justification, and others (such as the control page) could be done in scsi-hd as well. We should look into cleaning this up and making ufs-lu share a lot more code with scsi-hd; possibly even supporting -device scsi-hd with UFS devices. I am not going to ask you for a revert, but if this is not done before 8.2 is out, I will ask you to disable it by default in hw/ufs/Kconfig. In the future, please Cc the SCSI maintainers for UFS patches. Paolo Thanks for the comment. ufs-lu took most of its code from scsi-hd, so I completely agree that we should make scsi-hd code shareable to reduce code redundancy and make it better. I will fix it and get back to you soon. Thank you. Sincerely, Jeuk
Re: [PATCH 2/3] backends: Initial support for SPDM socket support
On Sat, Sep 16, 2023 at 1:19 AM Jonathan Cameron wrote: > > On Fri, 15 Sep 2023 21:27:22 +1000 > Alistair Francis wrote: > > > From: Huai-Cheng Kuo > > Great to see you taking this forwards! > > > > > > SPDM enables authentication, attestation and key exchange to assist in > > providing infrastructure security enablement. It's a standard published > > by the DMTF [1]. > > > > SPDM currently supports PCIe DOE and MCTP transports, but it can be > > extended to support others in the future. This patch adds > > support to QEMU to connect to an external SPDM instance. > > It supports way more that that these days. I'd just say 'multiple' > transports. > > > > > SPDM support can be added to any QEMU device by exposing a > > TCP socket to a SPDM server. The server can then implement the SPDM > > decoding/encoding support, generally using libspdm [2]. > > > > This is similar to how the current TPM implementation works and means > > that the heavy lifting of setting up certificate chains, capabilities, > > measurements and complex crypto can be done outside QEMU by a well > > supported and tested library. > > Is this sufficient for usecases beyond initial attestation flows? I believe so. The SPDM responder would be in charge of doing all of this. For the rest of the discussion the responder is the software on the other end of the QEMU socket. > How does measurement work for example? We need settings from the In a basic case the responder can generate measurement data. For example the responder can return digests of the firmware. Now there won't actually be "firmware", but the responder can still return measurement data. if you are trying to test an existing product, you could fake it and return the same values as a real device. Otherwise you could return example data. > emulated device to squirt into the SPDM agent so that it can be > encrypted and signed etc. > > Measurement reports often need to include the status of various config > space registers + any device specific additional stuff - not sure > what is defined for NVME but I suspect the list will grow, particularly > when tdisp is included. There are some things called out in the PCIe > state as must haves, like any debug features must be reported. So this is probably the hard part. How can the responder measurements change based on configuration values set by the host. Just for completeness, the idea would be the host would set some state in the NVMe controller for example. Then we would expect the measurement values to change. That's trickier, but we could extend the socket to communicate state like that. So when a measurement state changes in the QEMU model, we relay that to the responder. Which then changes the measurements > Also we need a way to mess with firmware revisions reported > as those are likely to be checked. That seems like something you can do via command line arguments or configuration settings to the responder. This is separate to QEMU > > I'm not sure that model will work with the spdm-emu approach. I don't think there is anything specifically in the socket approach that limits this from working. It comes down to passing information from the QEMU emulated device to the SPDM responder. That needs to be done either way. It probably is simpler to do if libspdm is included as part of QEMU, but that brings along other complexities. As you pointed out in my original RFC, the complexity of configuration a responder via the QEMU command line will be very difficult. I think it's simpler to keep the responder outside and QEMU and just pass any relevant data to the responder as required. > > Anyhow, I think we need to have gotten a little further figuring that > out before we merge a solution. I've been carrying this on the CXL > staging tree for a long time because I couldn't figure out a good solution > to the amount of information that needs to go between them. > > For those not familiar with the fun of libSPDM it is a pain to work with > which is why Huai-Cheng instead connected with the demo app. > > Any more luck getting a reliable build to work? Yes! libspdm is now packaged in buildroot: https://github.com/buildroot/buildroot/blob/master/package/libspdm/libspdm.mk You can now build libspdm with openSSL provided by your distro as you don't need to access any private openSSL APIs any more. The hope is we can continue to march towards including libspdm as a standard library in distros, which should make the entire process easier. > > > > > 1: https://www.dmtf.org/standards/SPDM > > 2: https://github.com/DMTF/libspdm > > > > Signed-off-by: Huai-Cheng Kuo > > Signed-off-by: Chris Browy > > Co-developed-by: Jonathan Cameron > > Signed-off-by: Jonathan Cameron > > [ Changes by AF: > > - Convert to be more QEMU-ified > > - Move to backends as it isn't PCIe specific > > ] > > Signed-off-by: Alistair Francis > Alistair, you sent this so I think your sign off should be last > + some indication of Wilfred's involvement
[PATCH] hw/ufs: Fix code coverity issues
From: Jeuk Kim
Fixed four ufs-related coverity issues.
The coverity issues and fixes are as follows
1. CID 1519042: Security issue with the rand() function
Changed to use a fixed value (0xab) instead of rand() as
the value for testing
2. CID 1519043: Dereference after null check
Removed useless (redundant) null checks
3. CID 1519050: Out-of-bounds access issue
Fix to pass an array type variable to find_first_bit and
find_next_bit using DECLARE_BITMAP()
4. CID 1519051: Out-of-bounds read issue
Fix incorrect range check for lun
Fix coverity CID: 1519042 1519043 1519050 1519051
Signed-off-by: Jeuk Kim
---
hw/ufs/lu.c| 16 +++-
hw/ufs/ufs.c | 10 +-
tests/qtest/ufs-test.c | 2 +-
3 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/hw/ufs/lu.c b/hw/ufs/lu.c
index e1c46bddb1..13b5e37b53 100644
--- a/hw/ufs/lu.c
+++ b/hw/ufs/lu.c
@@ -1345,13 +1345,12 @@ static void ufs_lu_realize(SCSIDevice *dev, Error
**errp)
return;
}
-if (lu->qdev.conf.blk) {
-ctx = blk_get_aio_context(lu->qdev.conf.blk);
-aio_context_acquire(ctx);
-if (!blkconf_blocksizes(&lu->qdev.conf, errp)) {
-goto out;
-}
+ctx = blk_get_aio_context(lu->qdev.conf.blk);
+aio_context_acquire(ctx);
+if (!blkconf_blocksizes(&lu->qdev.conf, errp)) {
+goto out;
}
+
lu->qdev.blocksize = UFS_BLOCK_SIZE;
blk_get_geometry(lu->qdev.conf.blk, &nb_sectors);
nb_blocks = nb_sectors / (lu->qdev.blocksize / BDRV_SECTOR_SIZE);
@@ -1367,10 +1366,9 @@ static void ufs_lu_realize(SCSIDevice *dev, Error **errp)
}
ufs_lu_brdv_init(lu, errp);
+
out:
-if (ctx) {
-aio_context_release(ctx);
-}
+aio_context_release(ctx);
}
static void ufs_lu_unrealize(SCSIDevice *dev)
diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c
index 0ecedb9aed..b73eb3deaf 100644
--- a/hw/ufs/ufs.c
+++ b/hw/ufs/ufs.c
@@ -258,7 +258,7 @@ static void ufs_irq_check(UfsHc *u)
static void ufs_process_db(UfsHc *u, uint32_t val)
{
-unsigned long doorbell;
+DECLARE_BITMAP(doorbell, UFS_MAX_NUTRS);
uint32_t slot;
uint32_t nutrs = u->params.nutrs;
UfsRequest *req;
@@ -268,8 +268,8 @@ static void ufs_process_db(UfsHc *u, uint32_t val)
return;
}
-doorbell = val;
-slot = find_first_bit(&doorbell, nutrs);
+memcpy(doorbell, &val, sizeof(val));
+slot = find_first_bit(doorbell, nutrs);
while (slot < nutrs) {
req = &u->req_list[slot];
@@ -285,7 +285,7 @@ static void ufs_process_db(UfsHc *u, uint32_t val)
trace_ufs_process_db(slot);
req->state = UFS_REQUEST_READY;
-slot = find_next_bit(&doorbell, nutrs, slot + 1);
+slot = find_next_bit(doorbell, nutrs, slot + 1);
}
qemu_bh_schedule(u->doorbell_bh);
@@ -838,7 +838,7 @@ static QueryRespCode ufs_read_unit_desc(UfsRequest *req)
uint8_t lun = req->req_upiu.qr.index;
if (lun != UFS_UPIU_RPMB_WLUN &&
-(lun > UFS_MAX_LUS || u->lus[lun] == NULL)) {
+(lun >= UFS_MAX_LUS || u->lus[lun] == NULL)) {
trace_ufs_err_query_invalid_index(req->req_upiu.qr.opcode, lun);
return UFS_QUERY_RESULT_INVALID_INDEX;
}
diff --git a/tests/qtest/ufs-test.c b/tests/qtest/ufs-test.c
index ed3dbca154..15d467630c 100644
--- a/tests/qtest/ufs-test.c
+++ b/tests/qtest/ufs-test.c
@@ -497,7 +497,7 @@ static void ufstest_read_write(void *obj, void *data,
QGuestAllocator *alloc)
g_assert_cmpuint(block_size, ==, 4096);
/* Write data */
-memset(write_buf, rand() % 255 + 1, block_size);
+memset(write_buf, 0xab, block_size);
ufs_send_scsi_command(ufs, 0, 1, write_cdb, write_buf, block_size, NULL, 0,
&utrd, &rsp_upiu);
g_assert_cmpuint(le32_to_cpu(utrd.header.dword_2), ==, UFS_OCS_SUCCESS);
--
2.34.1
Re: [PATCH 03/21] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
On 9/17/23 13:40, Mike Maslenkin wrote:
This is not introduced by this patch,
but looks like qemu_opts_del(opts) missed.
nice spot!
On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev wrote:
This patch creates above mentioned helper and moves its usage to the
beginning of parallels_open(). This simplifies parallels_open() a bit.
The patch also ensures that we store prealloc_size on block driver state
always in sectors. This makes code cleaner and avoids wrong opinion at
the assignment that the value is in bytes.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 65 +++
1 file changed, 38 insertions(+), 27 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 428f72de1c..1d5409f2ba 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1025,6 +1025,38 @@ static int parallels_update_header(BlockDriverState *bs)
return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
}
+
+static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
+ Error **errp)
+{
+char *buf;
+int64_t bytes;
+BDRVParallelsState *s = bs->opaque;
+Error *local_err = NULL;
+QemuOpts *opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp);
+if (!opts) {
+return -ENOMEM;
+}
+
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+return -EINVAL;
+}
+
+bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
qemu_opt_get_size_del returns uint64_t, so what's a reason to declare
"bytes" variable as int64_t
and then shift it to the right? I see here it can not be negative,
but it's a common to use signed values and not to add explicit check
before shifting to right In this file
I takes time to ensure that initial values are not negative.
Regards,
Mike.
apparently no specific reason. Just all variables here
dealing with offsets are int64_t. Will change.
Thanks,
Den
Re: [PATCH 06/21] parallels: refactor path when we need to re-check image in parallels_open
On 9/17/23 13:40, Mike Maslenkin wrote:
This patch generates a warning.
On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev wrote:
More conditions follows thus the check should be more scalable.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8f223bfd89..aa29df9f77 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1065,7 +1065,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-bool data_off_is_correct;
+bool need_check = false;
ret = parallels_opts_prealloc(bs, options, errp);
if (ret < 0) {
@@ -1133,11 +1133,12 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_bitmap = (uint32_t *)(s->header + 1);
if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-s->header_unclean = true;
+need_check = s->header_unclean = true;
}
-data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
- &data_start);
+need_check = need_check ||
+ !parallels_test_data_off(s, file_nb_sectors, &data_start);
+
../block/parallels.c:1139:18: warning: variable 'data_start' is used
uninitialized whenever '||' condition is true
[-Wsometimes-uninitialized]
need_check = need_check ||
^~
../block/parallels.c:1142:21: note: uninitialized use occurs here
s->data_start = data_start;
^~
../block/parallels.c:1139:18: note: remove the '||' if its condition
is always false
need_check = need_check ||
^
../block/parallels.c:1067:24: note: initialize the variable
'data_start' to silence this warning
uint32_t data_start;
^
= 0
1 warning generated.
Regards,
Mike.
wow! That is pretty much correct!
What compiler/OS you are using :)
Den
Re: [PATCH 18/21] parallels: naive implementation of parallels_co_pdiscard
I got a warning after this patch:
../block/parallels.c:541:25: warning: 'guarded_by' attribute only
applies to non-static data members and global variables
[-Wignored-attributes]
static int coroutine_fn GRAPH_RDLOCK_PTR
^
/Users/mg/sources/qemu/include/block/graph-lock.h:85:26: note:
expanded from macro 'GRAPH_RDLOCK_PTR'
#define GRAPH_RDLOCK_PTR TSA_GUARDED_BY(graph_lock)
^
/Users/mg/sources/qemu/include/qemu/clang-tsa.h:48:31: note: expanded
from macro 'TSA_GUARDED_BY'
#define TSA_GUARDED_BY(x) TSA(guarded_by(x))
Regards,
Mike.
On Fri, Sep 15, 2023 at 9:42 PM Denis V. Lunev wrote:
>
> * Discarding with backing stores is not supported by the format.
> * There is no buffering/queueing of the discard operation.
> * Only operations aligned to the cluster are supported.
>
> Signed-off-by: Denis V. Lunev
> ---
> block/parallels.c | 47 +++
> 1 file changed, 47 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 76aedfd7c4..83cb8d6722 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -537,6 +537,52 @@ parallels_co_readv(BlockDriverState *bs, int64_t
> sector_num, int nb_sectors,
> return ret;
> }
>
> +
> +static int coroutine_fn GRAPH_RDLOCK_PTR
> +parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
> +{
> +int ret = 0;
> +uint32_t cluster, count;
> +BDRVParallelsState *s = bs->opaque;
> +
> +/*
> + * The image does not support ZERO mark inside the BAT, which means that
> + * stale data could be exposed from the backing file.
> + */
> +if (bs->backing) {
> +return -ENOTSUP;
> +}
> +
> +if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
> +return -ENOTSUP;
> +} else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
> +return -ENOTSUP;
> +}
> +
> +cluster = offset / s->cluster_size;
> +count = bytes / s->cluster_size;
> +
> +qemu_co_mutex_lock(&s->lock);
> +for (; count > 0; cluster++, count--) {
> +int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
> +if (host_off == 0) {
> +continue;
> +}
> +
> +ret = bdrv_co_pdiscard(bs->file, cluster * s->cluster_size,
> + s->cluster_size);
> +if (ret < 0) {
> +goto done;
> +}
> +
> +parallels_set_bat_entry(s, cluster, 0);
> +bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
> +}
> +done:
> +qemu_co_mutex_unlock(&s->lock);
> +return ret;
> +}
> +
> static void parallels_check_unclean(BlockDriverState *bs,
> BdrvCheckResult *res,
> BdrvCheckMode fix)
> @@ -1409,6 +1455,7 @@ static BlockDriver bdrv_parallels = {
> .bdrv_co_create = parallels_co_create,
> .bdrv_co_create_opts= parallels_co_create_opts,
> .bdrv_co_check = parallels_co_check,
> +.bdrv_co_pdiscard = parallels_co_pdiscard,
> };
>
> static void bdrv_parallels_init(void)
> --
> 2.34.1
>
Re: [PATCH 03/21] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
This is not introduced by this patch,
but looks like qemu_opts_del(opts) missed.
On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev wrote:
>
> This patch creates above mentioned helper and moves its usage to the
> beginning of parallels_open(). This simplifies parallels_open() a bit.
>
> The patch also ensures that we store prealloc_size on block driver state
> always in sectors. This makes code cleaner and avoids wrong opinion at
> the assignment that the value is in bytes.
>
> Signed-off-by: Denis V. Lunev
> ---
> block/parallels.c | 65 +++
> 1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 428f72de1c..1d5409f2ba 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1025,6 +1025,38 @@ static int parallels_update_header(BlockDriverState
> *bs)
> return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
> }
>
> +
> +static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
> + Error **errp)
> +{
> +char *buf;
> +int64_t bytes;
> +BDRVParallelsState *s = bs->opaque;
> +Error *local_err = NULL;
> +QemuOpts *opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0,
> errp);
> +if (!opts) {
> +return -ENOMEM;
> +}
> +
> +if (!qemu_opts_absorb_qdict(opts, options, errp)) {
> +return -EINVAL;
> +}
> +
> +bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
> +s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
qemu_opt_get_size_del returns uint64_t, so what's a reason to declare
"bytes" variable as int64_t
and then shift it to the right? I see here it can not be negative,
but it's a common to use signed values and not to add explicit check
before shifting to right In this file
I takes time to ensure that initial values are not negative.
Regards,
Mike.
> +buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
> +/* prealloc_mode can be downgraded later during allocate_clusters */
> +s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
> + PRL_PREALLOC_MODE_FALLOCATE,
> + &local_err);
> +g_free(buf);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +return -EINVAL;
> +}
> +return 0;
> +}
> +
> static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>Error **errp)
> {
> @@ -1033,11 +1065,13 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> int ret, size, i;
> int64_t file_nb_sectors, sector;
> uint32_t data_start;
> -QemuOpts *opts = NULL;
> -Error *local_err = NULL;
> -char *buf;
> bool data_off_is_correct;
>
> +ret = parallels_opts_prealloc(bs, options, errp);
> +if (ret < 0) {
> +return ret;
> +}
> +
> ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
> if (ret < 0) {
> return ret;
> @@ -1078,6 +1112,7 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> ret = -EFBIG;
> goto fail;
> }
> +s->prealloc_size = MAX(s->tracks, s->prealloc_size);
> s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>
> s->bat_size = le32_to_cpu(ph.bat_entries);
> @@ -1117,29 +1152,6 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> s->header_size = size;
> }
>
> -opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp);
> -if (!opts) {
> -goto fail_options;
> -}
> -
> -if (!qemu_opts_absorb_qdict(opts, options, errp)) {
> -goto fail_options;
> -}
> -
> -s->prealloc_size =
> -qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
> -s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
> -buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
> -/* prealloc_mode can be downgraded later during allocate_clusters */
> -s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
> - PRL_PREALLOC_MODE_FALLOCATE,
> - &local_err);
> -g_free(buf);
> -if (local_err != NULL) {
> -error_propagate(errp, local_err);
> -goto fail_options;
> -}
> -
> if (ph.ext_off) {
> if (flags & BDRV_O_RDWR) {
> /*
> @@ -1214,7 +1226,6 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> fail_format:
> error_setg(errp, "Image not in Parallels format");
> -fail_options:
> ret = -EINVAL;
> fail:
> /*
> --
> 2.34.1
>
Re: [PATCH 06/21] parallels: refactor path when we need to re-check image in parallels_open
This patch generates a warning.
On Fri, Sep 15, 2023 at 9:41 PM Denis V. Lunev wrote:
>
> More conditions follows thus the check should be more scalable.
>
> Signed-off-by: Denis V. Lunev
> ---
> block/parallels.c | 19 ---
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 8f223bfd89..aa29df9f77 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1065,7 +1065,7 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> int ret, size, i;
> int64_t file_nb_sectors, sector;
> uint32_t data_start;
> -bool data_off_is_correct;
> +bool need_check = false;
>
> ret = parallels_opts_prealloc(bs, options, errp);
> if (ret < 0) {
> @@ -1133,11 +1133,12 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> s->bat_bitmap = (uint32_t *)(s->header + 1);
>
> if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -s->header_unclean = true;
> +need_check = s->header_unclean = true;
> }
>
> -data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
> - &data_start);
> +need_check = need_check ||
> + !parallels_test_data_off(s, file_nb_sectors, &data_start);
> +
../block/parallels.c:1139:18: warning: variable 'data_start' is used
uninitialized whenever '||' condition is true
[-Wsometimes-uninitialized]
need_check = need_check ||
^~
../block/parallels.c:1142:21: note: uninitialized use occurs here
s->data_start = data_start;
^~
../block/parallels.c:1139:18: note: remove the '||' if its condition
is always false
need_check = need_check ||
^
../block/parallels.c:1067:24: note: initialize the variable
'data_start' to silence this warning
uint32_t data_start;
^
= 0
1 warning generated.
Regards,
Mike.
