[PATCH 1/2] dm: virtio-blk: fix parameter err


Conghui Chen
 

Fix the truncate issue for virtio block parameter.

Signed-off-by: Conghui <conghui.chen@...>
---
devicemodel/hw/pci/virtio/virtio_block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/virtio/virtio_block.c b/devicemodel/hw/pci/virtio/virtio_block.c
index e499057ad..930a7f6ba 100644
--- a/devicemodel/hw/pci/virtio/virtio_block.c
+++ b/devicemodel/hw/pci/virtio/virtio_block.c
@@ -506,7 +506,7 @@ virtio_blk_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
- opts_tmp = opts_start;
+ opts_tmp = opts;
}
bctxt = blockif_open(opts_tmp, bident);
if (bctxt == NULL) {
--
2.25.1


Yu Wang
 

On Fri, Sep 16, 2022 at 04:35:28PM +0800, Conghui wrote:
Fix the truncate issue for virtio block parameter.

Signed-off-by: Conghui <conghui.chen@...>
---
devicemodel/hw/pci/virtio/virtio_block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/virtio/virtio_block.c b/devicemodel/hw/pci/virtio/virtio_block.c
index e499057ad..930a7f6ba 100644
--- a/devicemodel/hw/pci/virtio/virtio_block.c
+++ b/devicemodel/hw/pci/virtio/virtio_block.c
@@ -506,7 +506,7 @@ virtio_blk_init(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
- opts_tmp = opts_start;
+ opts_tmp = opts;
The logic is a little mess..

Let's try to seperated the iothread checking from the disk file opening.

if (strstr(opts, "nodisk") != NULL) {
dummy_bctxt = true;
} else {
bctxt = blockif_open(opts, bident);
if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
}

opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
}


}

}
bctxt = blockif_open(opts_tmp, bident);
if (bctxt == NULL) {
--
2.25.1


Conghui Chen
 

Hi Yu,

-----Original Message-----
From: Wang, Yu1 <yu1.wang@...>
Sent: Monday, September 19, 2022 10:41 AM
To: Chen, Conghui <conghui.chen@...>
Cc: acrn-dev@...
Subject: Re: [PATCH 1/2] dm: virtio-blk: fix parameter err


On Fri, Sep 16, 2022 at 04:35:28PM +0800, Conghui wrote:
Fix the truncate issue for virtio block parameter.

Signed-off-by: Conghui <conghui.chen@...>
---
devicemodel/hw/pci/virtio/virtio_block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/virtio/virtio_block.c
b/devicemodel/hw/pci/virtio/virtio_block.c
index e499057ad..930a7f6ba 100644
--- a/devicemodel/hw/pci/virtio/virtio_block.c
+++ b/devicemodel/hw/pci/virtio/virtio_block.c
@@ -506,7 +506,7 @@ virtio_blk_init(struct vmctx *ctx, struct pci_vdev
*dev, char *opts)
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
- opts_tmp = opts_start;
+ opts_tmp = opts;
The logic is a little mess..

Let's try to seperated the iothread checking from the disk file opening.

if (strstr(opts, "nodisk") != NULL) {
dummy_bctxt = true;
} else {
bctxt = blockif_open (opts, bident);
If the parameter has 'iothread', then it will be passed to blockif_open, it is not wanted.

We need to cut the 'iothread' from the parameter by strsep, and then pass the rest of parameter to blockif_open.
However, we can not cut it directly on opts, as strsep will replace the ',' with '\0' in opts. And move opts to point to the string after ','.
So we have to copy a new string by strdup, and do strsep on the new string.

if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
}

opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
}


}

}
bctxt = blockif_open(opts_tmp, bident);
if (bctxt == NULL) {
--
2.25.1


Yu Wang
 

On Mon, Sep 19, 2022 at 12:21:18PM +0800, Chen, Conghui wrote:
Hi Yu,

-----Original Message-----
From: Wang, Yu1 <yu1.wang@...>
Sent: Monday, September 19, 2022 10:41 AM
To: Chen, Conghui <conghui.chen@...>
Cc: acrn-dev@...
Subject: Re: [PATCH 1/2] dm: virtio-blk: fix parameter err


On Fri, Sep 16, 2022 at 04:35:28PM +0800, Conghui wrote:
Fix the truncate issue for virtio block parameter.

Signed-off-by: Conghui <conghui.chen@...>
---
devicemodel/hw/pci/virtio/virtio_block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/virtio/virtio_block.c
b/devicemodel/hw/pci/virtio/virtio_block.c
index e499057ad..930a7f6ba 100644
--- a/devicemodel/hw/pci/virtio/virtio_block.c
+++ b/devicemodel/hw/pci/virtio/virtio_block.c
@@ -506,7 +506,7 @@ virtio_blk_init(struct vmctx *ctx, struct pci_vdev
*dev, char *opts)
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
- opts_tmp = opts_start;
+ opts_tmp = opts;
The logic is a little mess..

Let's try to seperated the iothread checking from the disk file opening.

if (strstr(opts, "nodisk") != NULL) {
dummy_bctxt = true;
} else {
bctxt = blockif_open (opts, bident);
If the parameter has 'iothread', then it will be passed to blockif_open, it is not wanted.

We need to cut the 'iothread' from the parameter by strsep, and then pass the rest of parameter to blockif_open.
However, we can not cut it directly on opts, as strsep will replace the ',' with '\0' in opts. And move opts to point to the string after ','.
So we have to copy a new string by strdup, and do strsep on the new string.
Then let's try this way, put the regular path at the "if" section and
move the corner case into the "else" section.

if (strstr(opts, "nodisk") == NULL) {
opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
opts_tmp = opts;
}

bctxt = blockif_open (opts_tmp, bident);
if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
} else {
dummy_bctxt = true;
}



if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
}

opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
}


}

}
bctxt = blockif_open(opts_tmp, bident);
if (bctxt == NULL) {
--
2.25.1


Zhao, Yakui
 

On 2022/9/19 14:12, Yu Wang wrote:
On Mon, Sep 19, 2022 at 12:21:18PM +0800, Chen, Conghui wrote:
Hi Yu,

-----Original Message-----
From: Wang, Yu1 <yu1.wang@...>
Sent: Monday, September 19, 2022 10:41 AM
To: Chen, Conghui <conghui.chen@...>
Cc: acrn-dev@...
Subject: Re: [PATCH 1/2] dm: virtio-blk: fix parameter err


On Fri, Sep 16, 2022 at 04:35:28PM +0800, Conghui wrote:
Fix the truncate issue for virtio block parameter.

Signed-off-by: Conghui <conghui.chen@...>
---
devicemodel/hw/pci/virtio/virtio_block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/virtio/virtio_block.c
b/devicemodel/hw/pci/virtio/virtio_block.c
index e499057ad..930a7f6ba 100644
--- a/devicemodel/hw/pci/virtio/virtio_block.c
+++ b/devicemodel/hw/pci/virtio/virtio_block.c
@@ -506,7 +506,7 @@ virtio_blk_init(struct vmctx *ctx, struct pci_vdev
*dev, char *opts)
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
- opts_tmp = opts_start;
+ opts_tmp = opts;
The logic is a little mess..

Let's try to seperated the iothread checking from the disk file opening.

if (strstr(opts, "nodisk") != NULL) {
dummy_bctxt = true;
} else {
bctxt = blockif_open (opts, bident);
If the parameter has 'iothread', then it will be passed to blockif_open, it is not wanted.

We need to cut the 'iothread' from the parameter by strsep, and then pass the rest of parameter to blockif_open.
However, we can not cut it directly on opts, as strsep will replace the ',' with '\0' in opts. And move opts to point to the string after ','.
So we have to copy a new string by strdup, and do strsep on the new string.
Then let's try this way, put the regular path at the "if" section and
move the corner case into the "else" section.
if (strstr(opts, "nodisk") == NULL) {
opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
opts_tmp = opts;
It will be better to add some comments about the buffer pointed by opts_start is implicitly updated by strsep.
Based on the code it seems quite obscure as the opts_start/opts_tmp is assigned by using strdup(opt) and then opts_tmp is assigned assign.

}

bctxt = blockif_open (opts_tmp, bident);
if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
} else {
dummy_bctxt = true;
}


if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
}

opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
}


}

}
bctxt = blockif_open(opts_tmp, bident);
if (bctxt == NULL) {
--
2.25.1


Conghui Chen
 

-----Original Message-----
From: Wang, Yu1 <yu1.wang@...>
Sent: Monday, September 19, 2022 2:12 PM
To: Chen, Conghui <conghui.chen@...>
Cc: acrn-dev@...
Subject: Re: [PATCH 1/2] dm: virtio-blk: fix parameter err

On Mon, Sep 19, 2022 at 12:21:18PM +0800, Chen, Conghui wrote:
Hi Yu,

-----Original Message-----
From: Wang, Yu1 <yu1.wang@...>
Sent: Monday, September 19, 2022 10:41 AM
To: Chen, Conghui <conghui.chen@...>
Cc: acrn-dev@...
Subject: Re: [PATCH 1/2] dm: virtio-blk: fix parameter err


On Fri, Sep 16, 2022 at 04:35:28PM +0800, Conghui wrote:
Fix the truncate issue for virtio block parameter.

Signed-off-by: Conghui <conghui.chen@...>
---
devicemodel/hw/pci/virtio/virtio_block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/virtio/virtio_block.c
b/devicemodel/hw/pci/virtio/virtio_block.c
index e499057ad..930a7f6ba 100644
--- a/devicemodel/hw/pci/virtio/virtio_block.c
+++ b/devicemodel/hw/pci/virtio/virtio_block.c
@@ -506,7 +506,7 @@ virtio_blk_init(struct vmctx *ctx, struct
pci_vdev
*dev, char *opts)
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
- opts_tmp = opts_start;
+ opts_tmp = opts;
The logic is a little mess..

Let's try to seperated the iothread checking from the disk file opening.

if (strstr(opts, "nodisk") != NULL) {
dummy_bctxt = true;
} else {
bctxt = blockif_open (opts, bident);
If the parameter has 'iothread', then it will be passed to blockif_open, it is
not wanted.

We need to cut the 'iothread' from the parameter by strsep, and then
pass the rest of parameter to blockif_open.
However, we can not cut it directly on opts, as strsep will replace the ','
with '\0' in opts. And move opts to point to the string after ','.
So we have to copy a new string by strdup, and do strsep on the new
string.
Then let's try this way, put the regular path at the "if" section and
move the corner case into the "else" section.

if (strstr(opts, "nodisk") == NULL) {
opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
} else {
opts_tmp = opts;
}

bctxt = blockif_open (opts_tmp, bident);
if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
} else {
dummy_bctxt = true;
}
Sure, will update the logic, thanks.




if (bctxt == NULL) {
pr_err("Could not open backing file");
free(opts_start);
return -1;
}
}

opt = strsep(&opts_tmp, ","));
if (strcmp("iothread", opt) == 0) {
use_iothread = true;
}


}

}
bctxt = blockif_open(opts_tmp, bident);
if (bctxt == NULL) {
--
2.25.1