diff mbox

[FFmpeg-devel] Adding mkdir option for img2enc (3rd attempt)

Message ID 834b1299-267c-2010-aa2e-a23f3a458820@escribe.co.uk
State New
Headers show

Commit Message

Dr Alan Barclay Jan. 17, 2018, 10:56 a.m. UTC 
Hi,

Attached in a further patch - adding the error checks.

Derek - I don't think there is a functional downside to this 'mkdir' 
option being a default behaviour, but it would introduce a minor 
performance penalty (which users maybe don't want).

All comments and help appreciated.

Thanks and Regards,
Alan.


On 27/12/2017 16:01, Derek Buitenhuis wrote:
> Hi,
> 
> On 12/27/2017 12:27 PM, Dr Alan Barclay wrote:
>> Resending the two (git format-patch) patches, without the top lines
>> removed (which I thought I needed to do as some patch emails didn't seem
>> to have them).
>>
>> All comments and help appreciated.
> 
> [...]
> 
>> Subject: [PATCH 1/2] Move mkdir_p (renamed ff_mkdir_p) from hlsenc.c to
>>   utils.c.
>>
>> ---
>>   libavformat/hlsenc.c   | 35 +----------------------------------
>>   libavformat/internal.h |  7 +++++++
>>   libavformat/utils.c    | 33 +++++++++++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+), 34 deletions(-)
> 
> On a technical level, this patch looks OK.
> 
>> Subject: [PATCH 2/2] Adding mkdir option for img2enc.
>>
>> ---
>>   libavformat/img2enc.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
> 
> I'm not sure how others feel about the premise (mkdir in img2enc).
> I personally don't mind - though, maybe it should be default instead
> of an option? (Maybe a bad idea.)
> 
>> +        if (img->use_mkdir) {
>> +            char *temp_filename = av_strdup(filename);
>> +            const char *temp_path = av_dirname(temp_filename);
>> +            ff_mkdir_p(temp_path);
>> +            av_free(temp_filename);
>> +        }
> 
> This lacks error checks for av_strdup and ff_mkdir_p.
> 
> - Derek
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Comments

Carl Eugen Hoyos Jan. 17, 2018, 11:15 a.m. UTC | #1 
2018-01-17 11:56 GMT+01:00 Dr. Alan Barclay <alan@escribe.co.uk>:

> Attached in a further patch - adding the error checks.

Please merge this patch into your previous patch.

And please avoid top-posting here, Carl Eugen
Marton Balint Jan. 17, 2018, 7:56 p.m. UTC | #2 
On Wed, 17 Jan 2018, Dr. Alan Barclay wrote:

> Hi,
>
> Attached in a further patch - adding the error checks.
>
> Derek - I don't think there is a functional downside to this 'mkdir' option 
> being a default behaviour, but it would introduce a minor performance penalty 
> (which users maybe don't want).

I'd rather keep the mkdir functionality disabled by default, from a 
security standpoint I don't really like the idea of an ordinary muxer 
creating dirs, replacing parts of the path as default behaviour...

Thanks,
Marton

>
> All comments and help appreciated.
>
> Thanks and Regards,
> Alan.
>
>
> On 27/12/2017 16:01, Derek Buitenhuis wrote:
>> Hi,
>> 
>> On 12/27/2017 12:27 PM, Dr Alan Barclay wrote:
>>> Resending the two (git format-patch) patches, without the top lines
>>> removed (which I thought I needed to do as some patch emails didn't seem
>>> to have them).
>>> 
>>> All comments and help appreciated.
>> 
>> [...]
>> 
>>> Subject: [PATCH 1/2] Move mkdir_p (renamed ff_mkdir_p) from hlsenc.c to
>>>   utils.c.
>>> 
>>> ---
>>>   libavformat/hlsenc.c   | 35 +----------------------------------
>>>   libavformat/internal.h |  7 +++++++
>>>   libavformat/utils.c    | 33 +++++++++++++++++++++++++++++++++
>>>   3 files changed, 41 insertions(+), 34 deletions(-)
>> 
>> On a technical level, this patch looks OK.
>> 
>>> Subject: [PATCH 2/2] Adding mkdir option for img2enc.
>>> 
>>> ---
>>>   libavformat/img2enc.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>> 
>> I'm not sure how others feel about the premise (mkdir in img2enc).
>> I personally don't mind - though, maybe it should be default instead
>> of an option? (Maybe a bad idea.)
>> 
>>> +        if (img->use_mkdir) {
>>> +            char *temp_filename = av_strdup(filename);
>>> +            const char *temp_path = av_dirname(temp_filename);
>>> +            ff_mkdir_p(temp_path);
>>> +            av_free(temp_filename);
>>> +        }
>> 
>> This lacks error checks for av_strdup and ff_mkdir_p.
>> 
>> - Derek
>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>
>
> -- 
> Dr. Alan Barclay
> Electric Scribe Ltd.
> 118 Stanley Street
> Aberdeen AB10 6UQ, U.K.
> +44 1224 591779 office
> +44 7803 606485 mobile
> alan@escribe.co.uk
>
diff mbox

Patch

diff --git a/libavformat/img2enc.c b/libavformat/img2enc.c
index a8ee064396..9de2c8f356 100644
--- a/libavformat/img2enc.c
+++ b/libavformat/img2enc.c
@@ -116,9 +116,17 @@  static int write_packet(AVFormatContext *s, AVPacket *pkt)
             return AVERROR(EINVAL);
         }
         if (img->use_mkdir) {
+            const char *temp_path;
             char *temp_filename = av_strdup(filename);
-            const char *temp_path = av_dirname(temp_filename);
-            ff_mkdir_p(temp_path);
+            if (!temp_filename) {
+                return AVERROR(ENOMEM);
+            }
+            temp_path = av_dirname(temp_filename);
+            if (ff_mkdir_p(temp_path) == -1 && errno != EEXIST) {
+                av_log(s, AV_LOG_ERROR, "Could not create directory %s\n", temp_path);
+                av_free(temp_filename);
+                return AVERROR(errno);
+            }
             av_free(temp_filename);
         }
         for (i = 0; i < 4; i++) {