[Qemu-devel] Re: [COMMIT 707c0db] support colon in filenames

[Qemu-devel] Re: [COMMIT 707c0db] support colon in filenames

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

Anthony,

please revert this patch until Ram Pai provides a better version. It
causes too many problems. At least it

 - breaks -pflash somefile -snapshot
 - breaks mingw32 build due to redefined _open
 - uses PATH_MAX

rev3 does not fix any of the above.

At this chance: there is at least one indention issue remaining in
fill_token() which would be nice to fix while reworking the patch.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [COMMIT 707c0db] support colon in filenames

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

Jan Kiszka wrote:
> Anthony,
> 
> please revert this patch until Ram Pai provides a better version. It
> causes too many problems. At least it
> 
>  - breaks -pflash somefile -snapshot

Some more details on this: it's not just pflash that suffers, -snapshot
was broken for raw images in general. Declaring file access as a
protocol apparently has some unwanted side effects that first have to be
resolved (check also bdrv_open2 /wrt realpath).

>  - breaks mingw32 build due to redefined _open

Sorry, this obviously does not apply to the merged revision 2 but to
rev3 which I had applied locally.

>  - uses PATH_MAX
> 
> rev3 does not fix any of the above.
> 
> At this chance: there is at least one indention issue remaining in
> fill_token() which would be nice to fix while reworking the patch.
> 
> Jan
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [COMMIT 707c0db] support colon in filenames

[Ram Pai]

On Sun, 2009-07-05 at 23:47 +0200, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Anthony,
> > 
> > please revert this patch until Ram Pai provides a better version. It
> > causes too many problems. At least it
Jan,

Yes I am working on the next revision of the patch. 

> > 
> >  - breaks -pflash somefile -snapshot
> 
> Some more details on this: it's not just pflash that suffers, -snapshot
> was broken for raw images in general. Declaring file access as a
> protocol apparently has some unwanted side effects that first have to be
> resolved (check also bdrv_open2 /wrt realpath).

I do not entirely understand the side effect. I will see if I can figure
this out. It will certainly help if you can elaborate more?

> 
> >  - breaks mingw32 build due to redefined _open
> 
> Sorry, this obviously does not apply to the merged revision 2 but to
> rev3 which I had applied locally.
> 
> >  - uses PATH_MAX

PATH_MAX is not defined in mingw32 environment? 

> > 
> > rev3 does not fix any of the above.
> > 
> > At this chance: there is at least one indention issue remaining in
> > fill_token() which would be nice to fix while reworking the patch.
> > 

yes. noted.

Thanks,
RP

> > Jan
> > 
> 
> Jan
> 
 

[Qemu-devel] Re: [COMMIT 707c0db] support colon in filenames

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

Ram Pai wrote:
> On Sun, 2009-07-05 at 23:47 +0200, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Anthony,
>>>
>>> please revert this patch until Ram Pai provides a better version. It
>>> causes too many problems. At least it
> Jan,
> 
> Yes I am working on the next revision of the patch. 

I'm ok with keeping the change if you commit yourself to fix the
regressions quickly.

> 
>>>  - breaks -pflash somefile -snapshot
>> Some more details on this: it's not just pflash that suffers, -snapshot
>> was broken for raw images in general. Declaring file access as a
>> protocol apparently has some unwanted side effects that first have to be
>> resolved (check also bdrv_open2 /wrt realpath).
> 
> I do not entirely understand the side effect. I will see if I can figure
> this out. It will certainly help if you can elaborate more?

Start with running/debugging 'qemu raw.image -snapshot'. Also study the
codepath bdrv_open2 takes when snapshot is enabled. Specifically watch
out for realpath(). I'm not sure ATM why it is needed, but it used to be
applied on all file paths.

> 
>>>  - breaks mingw32 build due to redefined _open
>> Sorry, this obviously does not apply to the merged revision 2 but to
>> rev3 which I had applied locally.
>>
>>>  - uses PATH_MAX
> 
> PATH_MAX is not defined in mingw32 environment? 

It may not be defined in all environments (I've recently stumbled over
this with a patch, too), and it's possible that PATH_MAX < strlen(path).
Given that you can measure the input path length and the fact that the
output path cannot be larger, allocating this buffer from the heap is
really straightforward.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]